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

Add state support to match() #2175

Closed
wants to merge 1 commit into from
Closed

Add state support to match() #2175

wants to merge 1 commit into from

Conversation

insin
Copy link
Contributor

@insin insin commented Oct 6, 2015

This would make it easy to pass state (e.g. req.body) without having to duplicate match()'s history creation in order to pass a location object in.

match({ routes, location: req.url, state: req.body }, ...

@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

Ah, clever ;) Thanks for the PR, @insin!

I'm a little uncertain about adding state as an extra option to match though, if only because it's unclear which state takes precedence if the user actually does give me a real location object.

Is it too much to ask people to learn how to use the history.createLocation API?

@insin
Copy link
Contributor Author

insin commented Oct 6, 2015

I was going to use createLocation with 1.0.0-rc until I checked the latest source for the router and hsitory and noticed it's now a method; it felt a bit weird to create a object solely to use a utility method when the router would immediately be creating its own version of the same object and making the same method call with one fewer argument.

I could just create a single history object to reuse across all POST requests, so that works fine for me too if it's going to be the official way to pass state on the server.

@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

Ya, I agree the API is a bit awkward here. In the end, match is purely a convenience API. If I were you, I'd just copy the bits out of it that I need and not use match.

@timdorr
Copy link
Member

timdorr commented Oct 6, 2015

Maybe it would be best to retire match from the final API? It serves as a boilerplate reducer, but with the caveat of abstracting away some key APIs.

Since most of the magic isn't in the actual wrapping of history.match, but in the creation of the history object, perhaps that should be what is being wrapped. E.g., have a convenience function for creating a server-side history object:

import { createHistory } from 'react-router';
const history = createHistory()
const location = history.createLocation(req.url)
history.match(location, (error, redirectLocation, nextState) => { /* stuff... */ })

Then you can set up your location object however you'd like and get full access to any history APIs.

@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

a convenience function for creating a server-side history object

Hmm, that's actually a pretty great idea. My only concern is that people are going to start using it client-side, and that's not something we want to encourage. We need to make it extremely clear that the createHistory function in your example is only to be used server-side.

@timdorr
Copy link
Member

timdorr commented Oct 6, 2015

createMemoryHistory to indicate what's going on under the hood?

@insin
Copy link
Contributor Author

insin commented Oct 7, 2015

👍 on having a function for creating a server history object with all the appropriate configuration.

Perhaps a leaf from React's book:

var { DO_NOT_USE_ON_THE_CLIENT_OR_YOU_WILL_BE_FIRED } = require('react-router')
var { createHistory } = DO_NOT_USE_ON_THE_CLIENT_OR_YOU_WILL_BE_FIRED

(Or you could include a non-production check which logs a warning if we appear to be running on the client - people really like not having any of those in their console)

@timdorr
Copy link
Member

timdorr commented Oct 7, 2015

It's perfectly valid to create a memory history on the client. You lose a lot of benefits, but you won't explicitly break anything. Or get fired.

@mjackson
Copy link
Member

mjackson commented Oct 7, 2015

I think I would prefer it if, instead of another option to match we allow the location option to contain more information. That scales better IMO.

@mjackson
Copy link
Member

mjackson commented Oct 7, 2015

So, e.g.

match({
  routes,
  location: {
    pathname: '/the/path',
    state: req.body
  }
}, callback)

@insin
Copy link
Contributor Author

insin commented Oct 8, 2015

Looks like a good solution to me. Closing this in favour of whatever comes out of #2186

@insin insin closed this Oct 8, 2015
@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