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

Ensure match creates consistent histories #3089

Closed
wants to merge 2 commits into from
Closed

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Feb 17, 2016

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.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

Oh my gosh, what is that RRR issue doing?

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

Redirecting you back to '/' on the client side when you loaded up any route server side. Basically makes it unusable. But that's a userland error/fix. This is just for consistency.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

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. createHref.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

Yes, I want that to get to that implementation too. For now, this is a quick fix for the existing implementation.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

Well, no, the right thing to do if the user wants a stateful history is to just pass that into match, rather than passing in both history and location. I don't think we should be explicitly doing any state management inside match. Our desire to not do this is exactly why we didn't merge #2680, and instead kept around history.createLocation at all as an API.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

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.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

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 createMemoryHistory, as the docs say to do.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

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 history.createLocation here at all.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

Actually, we shouldn't need to use createLocation at all, since this is cleaning up the location ahead of time. One sec...

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

There we go. No more createLocation 😄

@taion
Copy link
Contributor

taion commented Feb 17, 2016

No, I mean, the change to drop createLocation is exactly the change we rejected in #2680 for being a move in the wrong direction strategically.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

Yeah, and that's a much larger discussion with how to split out state management and location creation/processing parts of history. (That can most likely be solved with some sort of LocationManager that gets passed into createHistory() to handle location creation and can get enhanced instead of the history instance.)

But no work has been done in that regard. We only know that createLocation will change (by removal or relocation) at some point. So, we should stop using it. This change simplifies our code to only have one path. Once the new API materializes, we can decide to either refactor around a new single code path or, worst case, add back branching depending on input. But we don't know what that will be yet, so we shouldn't try to predict where things are going to go. That kind of behavior tends to box you into an architectural corner pretty quickly (or worse, analysis paralysis).

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

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

@taion
Copy link
Contributor

taion commented Feb 17, 2016

I don't think createLocation will change – I think we'll pull out the relevant bits like createHref and createLocation into a purely stateless URL manager object, one that won't be listen-able; hence why I don't think we should make this change for the "normal" use case.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

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.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

I'd rather we just hide listen on the automatically-created history, actually. It's going to make life harder if people assume they're getting listen-able objects in contexts where they may not.

@ryanflorence
Copy link
Member

I don't have my head around everything going on here but:

I think our ultimate goal with the SSR API is to not require a history at all

Yes. What's the use-case for providing a custom history on the server?

I'd rather we just hide listen on the automatically-created history, actually.

Also sounds good.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

I don't know why you'd provide a custom history when doing SSR; I really think the original react-router-redux issue is just very confused.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

What's the use-case for providing a custom history on the server?

You provide an enhanced history. Although that doesn't have any practical purpose that I know of.

@taion
Copy link
Contributor

taion commented Feb 17, 2016

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.

@timdorr
Copy link
Member Author

timdorr commented Feb 18, 2016

OK, opening up this can of worms again...

Regardless of how users use renderProp, internally we are creating an inconsistent history instance that doesn't follow our own docs. That's all I'm trying to change in this PR. I know you all have grand plans for a history-less match, but that's not what I'm trying to do here. Just fixing a small internal inconsistency...

As for blocking access to router.listen, I'm on board with that as well. Do we want to just hide it from enumeration or wrap it with a warning? That would be a separate PR.

@taion
Copy link
Contributor

taion commented Feb 18, 2016

I'd rather just update the line in the docs – that we use a createMemoryHistory for server-side rendering is purely an implementation detail. The user shouldn't be aware of the existence of the history at all, except in very special edge cases.

@timdorr
Copy link
Member Author

timdorr commented Feb 18, 2016

OK, then this is dead.

What about hiding router/history.listen?

@timdorr timdorr closed this Feb 18, 2016
@timdorr timdorr deleted the consistent-history branch February 18, 2016 02:33
@taion
Copy link
Contributor

taion commented Feb 18, 2016

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.

@timdorr
Copy link
Member Author

timdorr commented Feb 18, 2016

Opened up remix-run/history#232 to discuss possible changes to history to support a history-less match.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants