-
Notifications
You must be signed in to change notification settings - Fork 960
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
Drop the default location in createMemoryHistory #231
Comments
Can you expound a bit? The default location is just there so you don't have to |
Memory histories are meant for the server, where there should always be an available URL. It's very easy to overlook the createMemoryHistory arg, because it functions without it. This results in creating an inconsistent history object with things like match on Router. I've had several users run into this exact problem with react-router-redux. It seems to me like it should either a) require that location arg, or b) create an empty history if it is not provided. I think having a convenience for testing isn't really in the best interest of the end user. You can wrap that as createTestHistory if it's really useful. |
The purpose of memory history is mainly for testing and to serve as a We've got to get away from people using it on the server. History just
|
That was the motivation of #232. The problem is this – server-side rendering with React Router requires most of a history. Specifically, this object has to:
IMO the most pragmatic solution is to just use an actual history here. The alternative in #232 has some conceptual benefits, but then what do you do for enhancers? It wouldn't make much sense to duplicate all of them, or else have them all branch on whether they got a history v just a location creator. |
I want RR to be able to take a URL and possibly an extra |
href generation is architecturally tied into the history right now. Query stringification is configured via Looking to the future, this also lets us set up something like a |
That's a great summary of all the touch points where we're currently coupled. |
Especially with respect to enhancement, this is a good thing from an API perspective – it means I can do all the configuration for path generation by just enhancing the history, and not have to worry about having separate, potentially non-synced configs. Which is to say that there are really three options:
The biggest barrier is the browser history v hash history thing – without duplicating the config in the router level for whether to prefix And I think that really speaks to the strength of this coupling, in that it allows for a much more user-friendly API. |
remix-run/react-router#3394 has made me mostly come across to @mjackson's view here. I think there's an easier solution than all this, though – why don't we make something like a We end up not needing to duplicate enhancers, and we avoid most of the confusion involved in passing in an irrelevant history object when doing server-side rendering, while still preserving the rest of the contract related to generating URLs. |
History is used to drive the state of the router in react-router, so essentially you always need a history and always need a location because without it the router does nothing. It's just a artefact of the approach they took in bundling in history. The reverse methodology is also possible---only update history via the router, and only accept changes from history.listen in the browser. In this case location/state is stored inside the router itself, instead of being reusing the tools given by History. |
v4 doesn't do any query string, location, or path parsing or creation. So the router should be able to be completely decoupled from history at this point and just use it as a dep in client-side environments. v4 histories should never be used on the server. |
createMemoryHistory
uses a default location of'/'
when not provided an initial location. This will likely cause unexpected behavior.We should not provide a default history and should throw if one is not provided (since not having one is probably going to break a bunch of other stuff).
The text was updated successfully, but these errors were encountered: