Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore entries attribute in MemoryHistory interface #939

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Donorlin
Copy link

Would feature fix this #828

This PR gives back the feature of MemoryHistory.entries which was available in version 4

Many people are dependent on this feature, mainly because it gives more control over memory history itself.

For example, we are running SPA in the SSR (company historical reasons). This means the SPA state is lost whenever a site is reloaded. In version history@4 we are able to save entries from the history object and pass it to initialEntries of the MemoryRouter to seed it back to its previous state (controlled behavior).

In react-router@6 we are still able to access the MemoryHistory object with UNSAFE_NavigationContext, but not its entries. So we are not able to implement a controlled MemoryRouter.

If it was intentional to remove entries for API reasons, close this PR.

Thank you for your response.

@Nantris
Copy link

Nantris commented Mar 10, 2022

@mjackson could we get some clarity on this? We rely on being able to access history.entries and I know we're not alone from reading #828.

Is there some reason for removing this? Can we approve this PR to add it back? There was no indication this interface was going to be removed and so no reason people would have been hesitant to rely on it - but now we're locked in the past on version 4.x unless either:

  • A: This PR is merged
  • B: We branch history and thereby lose access to future updates without extensive work

@Nantris
Copy link

Nantris commented Mar 10, 2022

It's also not mentioned as something that was removed, as far as I can tell:

If there's no hard blockers here, I really hope we can get this merged.

@Nantris
Copy link

Nantris commented Mar 15, 2022

Could we get a reply on this, even if the answer is no to merging? We need clarity on this.

@mjackson

@Nantris
Copy link

Nantris commented Mar 30, 2022

Could we just get a comment on why it was removed? Please?

It would be awful to go through the work of forking this library to expose entries just to find out there's some good reason it's not exposed anymore. As far as I can tell, it was just removed without mention or reason, but I really don't want to stake hours of time on that and be wrong.

@Nantris
Copy link

Nantris commented Mar 30, 2022

@chaance sorry to tag you, but I hoped maybe you could provide clarity here as substantial recent contributor.

@Nantris
Copy link

Nantris commented Apr 21, 2022

Can anyone provide clarity on this? Why not merge this PR?

@chaance
Copy link
Contributor

chaance commented Apr 22, 2022

@slapbox We've been busy with other priorities, and Michael has to review/approve API changes. These things sometimes take time. If you can't wait, there's always patch-package.

@Nantris
Copy link

Nantris commented Jul 14, 2022

Friendly bump.

@Nantris
Copy link

Nantris commented Aug 4, 2022

Friendly bump. I'd love to get our project up to date and using the latest react-router, but this is a blocker for us and the PR is ultra-minimalist. I hope it can be approved soon.

@Nantris
Copy link

Nantris commented Aug 26, 2022

Friendly bump. We remain hesitant to use patch-package in case there's some reason that the entries property was removed from MemoryHistory, although that seems unlikely since it's not in the patch notes.

@JarekToro
Copy link

Bump

@Nantris
Copy link

Nantris commented Nov 22, 2022

I've given up hope on React Router at this point.

@Donorlin
Copy link
Author

It looks like this package is not even used by react-router anymore. Pardon me if I am wrong. I am using only github for browsing router packages.
All history implementation is now here @remix-run/router and createMemoryRouter is here packages/router/history.ts#L214.
Sadly still no entries are exported even though it is more or less the same implementation.

Later today, maybe tommorow, I will recreate this PR for missing entries. Wish us luck.

@Donorlin
Copy link
Author

looks like there already was a PR with this change remix-run/react-router#10499 but got closed.

Apparently this is viewed as a new feature and it should go through their Open Development Process.

Please make some polite noice at this open discussion for this "new" feature remix-run/react-router#9853

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants