Drop the use of createLocation

[taion]

No description provided.

[taion]

Might as well merge the fixes to the match docs.

[taion]

Always fire the listener, but...

[taion]

The last commit should be squashed - I have it separate for now to highlight this interesting new edge case that comes up.

[mjackson]

So, we're doing all of this just for the server-side match API, right? What if the router just reached into history's internals and required history/lib/createLocation for now? Would that be terrible?

[taion]

@mjackson It's not enough in the general case - you need to go through the logic in useQueries or useBasename. The creation of the location object has to go through the history.

[taion]

One more commit we need to squash, to make sure we actually are always running through all the history enhancers (the test case is contrived, but it fails without the change, so that's good enough for me).

[mjackson]

Good point. But my intention never was for people to construct a history
object on the server. Servers are stateless, so I'd prefer to have a
server-side API that doesn't require people to create one.

On Tue, Dec 8, 2015 at 3:50 PM Jimmy Jia notifications@github.com wrote:

@mjackson https://github.com/mjackson It's not enough in the general
case - you need to go through the logic in useQueries or useBasename. The
creation of the location object has to go through the history.

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

[taion]

I don't know that we have a good way to get around it. With e.g. the current router API, <Link>s depend on history.createHref to create their hrefs.

I like the idea of not requiring a history for server-side rendering, but we'd have to rethink a lot of assumptions to get there, and I don't know that we'd end up somewhere where we didn't just invent a "stateless history" that just supports creating hrefs and such.

[timdorr]

This is all going to change a lot when useRoutes is refactored. @mjackson had proposed something like this a while ago, which I liked:

history.listen(location => {
  match(location, routes, (error, state) => {
    render(...)
  })
})

And I assume that would be how the internals of RouterContext will eventually look. And for SSR, it would be a simple swap:

app.use((req, res) => {
  match(req.url, routes, (error, state) => {
    render(...)
  })
})

[mjackson]

Generally, stuff like query parsing is already done by the time the request hits the router on the server. In express, for example, people can do:

app.use(function (req, res) {
  match({
    routes,
    location: {
      pathname: req.path,
      query: req.query,
      state: req.body // or session state, or whatever
    }
  }, ...)
})

[taion]

@timdorr I thought I was taking things in that direction in #2676 by switching it from location to path in match, but as you pointed out, that sort of API would exclude using state per e.g. #2175.

[timdorr]

Sorry, hit enter too soon.

In that case, then the server wouldn't require a history object at all. We only require it right now because match hasn't been refactored.

[taion]

@mjackson I actually mean converting the query to a search - that has to go through useQueries right now, per the latest commit on this PR.

@timdorr You don't need a history to run the route matching, but you can't render a <Link> properly without a history because of https://github.com/rackt/react-router/blob/master/modules/Link.js#L118.

[timdorr]

Yeah, that's true. But it doesn't mean we have to fake a navigation to a location in history just to get it to work server side. That seems like we're missing something on history.

[mjackson]

In general it just feels wrong to be using a history object at all in a stateless environment. I'd like to find a way forward that doesn't require us to do that.

Right now, useQueries and useBasename are both concerned with doing things that are commonly already done in server-side routers before they hit react router. The only things that we use history objects for that maybe we shouldn't is for these create* functions. e.g. <Link> needs to ask a history object how to create its href. But maybe it shouldn't do that.

[taion]

@timdorr

It's tricky - you can't get the behavior demonstrated in the PR with entries, because the createMemoryHistory function doesn't have access to the "wrapped" push and replace methods, so there isn't a good way for that function to use the logic in any history enhancers wrapping it.

We could introduce a new API point on history to support this, but I don't think the extra API surface area in that case pays for itself. I don't really see anything wrong with calling history.push just to run the location descriptor through the transformations in the enhancers.

[taion]

@mjackson

I think the way we would do that would be to factor out the "location creation" parts from the "navigation management" parts of history. Essentially, have an immutable object that handles locations and location conversion (i.e. the current logic in createPath, createHref, createLocation, &c.), and another part wrapping that core thing that handles push, pull, &c. This would make e.g. query and basename support largely orthogonal from hash v browser history.

I feel like this is a bigger refactoring than is practical at the moment, though. I also don't really know how this would work w/e.g. createHashHistory, which needs to modify how hrefs are constructed.

To the extent that we want to make server-side rendering less coupled with history, though, I think we should keep around history.createLocation; i.e. the non-documentation commits on this PR would be a step in the wrong direction.

Given that, what do you think about keeping around history.createLocation?

[mjackson]

what do you think about keeping around history.createLocation?

If anything I think this PR has me convinced that we need to keep it around at least a little bit longer. But I'd really like to find better ways to do it in the future.

[taion]

Okay - and to be clear, to avoid breaking users, it has to be the instance method, because right now it's explicitly part of this API that match will do things like parse queries and strip out the basename for you.

I'm going to close this PR and re-submit with just the doc fixes from the first commit, so it's obvious that we didn't merge these changes.