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

Drop the default location in createMemoryHistory #231

Closed
timdorr opened this issue Feb 17, 2016 · 11 comments
Closed

Drop the default location in createMemoryHistory #231

timdorr opened this issue Feb 17, 2016 · 11 comments

Comments

@timdorr
Copy link
Member

timdorr commented Feb 17, 2016

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

@mjackson
Copy link
Member

mjackson commented Mar 9, 2016

Can you expound a bit? The default location is just there so you don't have to createHistory([ '/' ]) every time you wanna do a test.

@mjackson mjackson mentioned this issue Mar 9, 2016
@timdorr
Copy link
Member Author

timdorr commented Mar 9, 2016

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.

@mjackson
Copy link
Member

mjackson commented Mar 9, 2016

The purpose of memory history is mainly for testing and to serve as a
reference implementation for the other histories. It's basically a way for
me to test API.

We've got to get away from people using it on the server. History just
doesn't make sense in that context.
On Tue, Mar 8, 2016 at 11:01 PM Tim Dorr notifications@github.com wrote:

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.


Reply to this email directly or view it on GitHub
#231 (comment).

@taion
Copy link
Contributor

taion commented Mar 9, 2016

@mjackson

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:

  • Support createHref to allow generating link hrefs on the server
  • Support converting location descriptors to locations (because match takes a location descriptor, but the router requires a full location object to match)
  • Support essentially the same API for enhancing this object to e.g. customize the query handling behavior

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.

@mjackson
Copy link
Member

mjackson commented Mar 9, 2016

I want RR to be able to take a URL and possibly an extra state object as its input and that's it. RR should be able to generate hrefs all on its own and do everything else with just that info. Otherwise they're too tightly coupled.

@taion
Copy link
Contributor

taion commented Mar 9, 2016

href generation is architecturally tied into the history right now. Query stringification is configured via useQueries, basename configuration is configured on useBasename, and hash history v browser history also affects the URL generated. The router can't figure out how to format hrefs without reference to the history, or at least "most of" a history per #232.

Looking to the future, this also lets us set up something like a useNamedRoutes history enhancer and only do everything in one place (e.g. history.push({ name: 'foo', params })), and still have href generation just work.

@mjackson
Copy link
Member

That's a great summary of all the touch points where we're currently coupled.

@taion
Copy link
Contributor

taion commented Mar 10, 2016

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:

  1. Require the user to separately configure e.g. query stringification on the history and the router
  2. Have something like [WIP] Extract location creation to "location providers" #232 that's most-of-a-history (and I guess migrate the enhancers to expose e.g. encodeLocation/decodeLocation methods instead of wrapping push and replace directly)
  3. Migrate enhancers like useQueries out of this library entirely, and have them just live in the router (except even this isn't enough, because hash history and browser history create different hrefs)

The biggest barrier is the browser history v hash history thing – without duplicating the config in the router level for whether to prefix hrefs with a #, the router needs to ask the history for how hrefs should be generated.

And I think that really speaks to the strength of this coupling, in that it allows for a much more user-friendly API.

@taion
Copy link
Contributor

taion commented Apr 28, 2016

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 createDummyHistory or createStubHistory or whatever that implements most of the history API, but just omits stateful methods like listen, push, and replace?

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.

@KaySackey
Copy link

We've got to get away from people using it on the server. History just doesn't make sense in that context.

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.

@mjackson
Copy link
Member

mjackson commented Sep 3, 2016

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.

@mjackson mjackson closed this as completed Sep 3, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants