-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Ensure match creates consistent histories #3089
Conversation
Oh my gosh, what is that RRR issue doing? |
Redirecting you back to |
This doesn't immediately strike me as being correct – I think our ultimate goal with the SSR API is to not require a history at all, and to only require the bits relating to building URLs with e.g. |
Yes, I want that to get to that implementation too. For now, this is a quick fix for the existing implementation. |
Well, no, the right thing to do if the user wants a stateful history is to just pass that into |
I think this takes us meaningfully farther away from that. It really looks like reactjs/react-router-redux#284 is just doing something incorrect. Syncing the history to the store in the context of server-side rendering doesn't really make sense to me, and even if it were somehow necessary, I don't think we should be handling that case here. |
This is for those that don't pass in a history and just a location (the vast majority of real-world cases, I would imagine). It's ensuring the internally-created history instance is consistent with the provided location. For those creating their own history, they would pass the location into |
I don't see why we want that, though. In this context, we specifically do not want people to be able to listen to this history object, to make it easier to transition it into something fully stateless later on. Otherwise we'd be better off using the code from #2680 and not relying on |
Actually, we shouldn't need to use |
There we go. No more |
No, I mean, the change to drop |
Yeah, and that's a much larger discussion with how to split out state management and location creation/processing parts of But no work has been done in that regard. We only know that |
Note: I've suggested we drop the default location from memory histories: remix-run/history#231 It's problematic and is unexpected. We actually had a test teetering on the edge of failure as a result of this: https://github.com/reactjs/react-router/pull/3089/files#diff-db41db2de1c0357baa6227d912609546L110 |
I don't think |
Again, that's all postulation. I'd rather make a pragmatic simplification of the code now that will make our jobs easier in the future. |
I'd rather we just hide |
I don't have my head around everything going on here but:
Yes. What's the use-case for providing a custom history on the server?
Also sounds good. |
I don't know why you'd provide a custom |
You provide an enhanced history. Although that doesn't have any practical purpose that I know of. |
Ah, sorry, you're right – custom enhancers for e.g. query string handling right now, and possibly named route support in the future. But it's still a bit of an edge case. |
OK, opening up this can of worms again... Regardless of how users use As for blocking access to |
I'd rather just update the line in the docs – that we use a |
OK, then this is dead. What about hiding |
As a quick precautionary thing, ping @mjackson – is this consistent with what you're intending? I'm just trying to work off your comments earlier regarding dropping the need for histories for SSR in a pragmatic way. |
Opened up remix-run/history#232 to discuss possible changes to history to support a history-less |
Ran into this with react-router-redux: reactjs/react-router-redux#284
If users don't provide a history they create themselves, the history created by
match
doesn't have the same entries as the location provided. This can lead to all sorts of funky behavior.We'll probably also want to drop the default location provided by
createMemoryHistory
and throw if an initial location isn't provided.