-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Allow transition hooks access to a context object you can specify when you create the router. #590
Conversation
interesting, the basic issue is that in regular component hooks you have access to @mjackson thoughts? |
I would love this pull request. I have the exact same use case. Want to use a context object (from yahoo/dispatchr) to check if the stores have a valid state. |
Yeah, I've been thinking about this too:
What's really happening is that react is just a view layer that the router lives outside of, and you need some sort of application-level dependency injection of app objects (module system and singletons are insufficient because of circular dependencies). So we have two options that I can think of:
I'm leaning toward (1). @mjackson thoughts? |
We already have the state arg. Why not just use that and make it a public API? Everybody seems to want to put stuff on it anyway.. |
So something like this? var appStuff = { foo: 'bar' };
var router = Router.create({
routes: routes,
extraState: appStuff
});
Router.run(routes, (Handler, state) => {
state.extraState; // available here
});
var SomeHandler = React.createClass({
statics: {
willTransitionTo (transition, params, query, state) {
state.extraState; // available here
},
// or totally change the signature since params/query become redundant
willTransitionTo (transition, state) {
state.params; state.query;
state.extraState; // available here
}
}
}); |
That would solve the problem indeed and looks nicer than the custom solution. Does this already work? |
Ya, similar to that. You don't need the Feels pretty sloppy tho. "We don't really have an API for this, so just pin whatever props you want on the big bag of state". |
@rpflorence so, I was thinking more like this: var appStuff = { foo: 'bar' };
Router.run(routes, (Handler, state) => {
state.extraState = appStuff; // put whatever you want on the state here
}); As for the transition hooks, I personally like the |
That callback function is called many more times than will be required to assign the extraState (once is plenty) which to me makes it seem like maybe that isn't the right place to put it. |
@bobpace so your "context" isn't state/URL specific stuff, but more on the page/request level? |
Yes that's right. My specific use case is yahoo's fluxible app with a plugin to use react-router. The plugins allow you an opportunity to plug various contexts with services that will be needed in the application. For me, this is where I create the react-router instance and pass their fluxContext object into Router.create so it can be handed to the static willTransition methods. On the server I have middleware that creates this flux context per request and dehydrates itself (potentially after dispatching actions server side) to the client to be rehydrated. |
I quite like the API that @bobpace uses in this PR — it gives you a formal place to put this kind of contextual data. That said, it does seem like the ability to modify this data per URL (e.g. each time the router runs the callback and re-renders) is nice, though my use case is similar (passing a per-page/request flux object around), and the "just stick it anywhere on the state" strategy opens up the possibility for key collisions down the road. |
Had same issue mentioned #302 (comment) -- this PR looks great. Really helpful for passing request specific variables around in node. Currently for auth, I'm passing req/req.currentUser when I resolve all my routes, and passing currentUser through application state. But not available in hooks. |
+1 , my use case is very similar to @ndreckshage |
Is there anything wrong with passing in context as a prop on Handler in Router.run(), then passing this.props.context to each subsequent RouteHandler? It feels hack but so far it's working for me. (I'm using Fluxible-app's context) |
@kilometers are you accessing that context from within the willTransitionTo or willTransitionFrom hooks? In willTransitionFrom you get handed the element you are moving away from so you could probably pull it through there but willTransitionTo does not hand you the element so you have no real way to get at any contextual data (unless you scope it with a closure around your entire react class definition, but that does not work for request specific data). I do the same thing you are doing by passing along the componentContext as a prop to the Handler in Router.run() as a means of getting the flux context through the rest of the application, but still need this change to get it to those static hooks. |
@bobpace Thanks for the clarification. No, I haven't used willTransitionTo yet. I'm just trying to learn by following these issues and I got confused about the need for a more integrated way to pass context |
@mjackson I'd like to make a decision here. In my own apps I have an app container that my transition hooks can access "context" information, but I'd be okay with something like this: var router = Router.create({
routes: someRoutes,
getTransitionContext () {
return something;
}
}); And then before every transition we'd call willTransitionTo (transition, params, query, context) {
transition.context;
// or
context;
} |
Had the same issue using Yahoo's I'd be ok with having a |
is there any updates about this PR? |
@NourSammour , you can get around it by |
@FoxxMD this way didn't work :( |
Same problem with fluxible, any updates on this? |
Would also love to see progress here. Anything I can help with? |
@mjackson @ryanflorence Any news? Maybe nothing has been settled yet, but are you talking about this? Or is the best bet to just fork and use that for now? |
I've been using @bobpace's fork (https://github.com/bobpace/react-router/tree/transitionContext), and it's worked out nicely. But it's a bit behind (v0.11.6 versus the latest v0.12.0)... |
@nicolashery I rebased against the latest master, squashed the commits and force pushed it back up so it's current again. :) |
👍 |
For everybody asking about this: Take a look at #1158
With the new version this PR becomes obsolete. |
Thanks @johanneslumpe. Guess I'll keep using this branch until 1.0 is released. The new direction makes sense. Helpful to have the heads up. |
I'm having issues integrating this branch into my Browserify build:
package.json:
Anyone else having this issue? The |
@idolize I think this is because the source is written in ES6 (and transpiled to ES5 before pushing to npm). If you use the fork, you'll need to run the build yourself and require with: var Router = require('react-router/build/npm/lib'); You can also use my branch, in which I checked in the build: https://github.com/nicolashery/react-router/tree/example-isomorphic-one |
@nicolashery I thought that may be the issue. Thanks for the tip! |
+1 |
1 similar comment
+1 |
+1 |
@jameslk if your environment does support ES6, however, such as with browserify and babel, this still won't run out of the box. you can add the following to the branch's package.json to get browserify to use the proper transform ...
Edit: Actually idk if this works or not. It builds fine but run time throws a ton of errors. :/ Oh, the branch already has the built files in it. Just need to |
What this pull request boils down to is trying to force the router into being an application container, which is not something we want to be. So you can either 1) embrace your need for an application container since you aren't using singletons or 2) read on ... 1.0 beta has been released that has two changes to get you what you need:
So here's what it could look like: // use the JSX API
<Route component={App} contextThing={context} onEnter={handleEnter}/>
// or use what we turn the JSX API into
var myRoute = {
component: App,
onEnter: handleEnter,
// add any properties you want
contextThing: context,
};
function handleEnter (nextState, transition) {
// and now `this` is the route
this.contextThing.whatever();
} Seems good? If you don't want to add it to every route manually, you can use some javascript in whatever fashion you want to not have to repeat yourself. |
@ryanflorence I understand where you are coming from and it's an acceptable perspective. The only thing is, that the new way of doing this requires people to always re-create their routes on each request. I assume you'd recommend to only ever re-create the route that needs the context object and then just add children to it, which might not have to be re-created each time? |
either recreate your routes (they're just lightweight objects) or use an application container, we aren't going to be an app container. |
Oh, I forgot to mention, you can also render a router inside of another component, and have that thing set context or pass in props in var App = React.createClass({
render: function () {
// now you're in a render cycle and can do whatever you would have done w/o the router
// like set context, or maybe some of this stuff...
return <Router
createElement={(Component, props) => {/* inject props to route components*/})}
>
<Route onEnter={() => /* have access to your stuff here, etc. */}/>
</Router>
}
}); |
With React Router 0.13.3, you can take advantage of the synchronous nature of Router and supply your per-request transition context through a global variable just before creating your router like so: function HandleRequest(request, response, next) {
const context = ...;
global.appContext = context; // global.appContext will be defined in willTransitionTo callback
const router = Router.create({
routes: routes,
location: request.url
}
});
router.run(...);
global.appContext = undefined;
} And of course on the client-side, you can just set |
@ryanflorence sorry, too much different points/comments in this thread. Regarding ReactRouter version |
Sorry if I'm missing something here, but I'm slightly confused about how this would work. Let's say a user logs in on the client-side and we store relevant info in localStorage.user as well as do UserStore.setLoggedInUser(user). Now I do a page reload - how would I know the user is logged in on the server-side? If you create a new flux instance for every request, wouldn't all of the stores be in some initial empty state, which means the value we wrote when calling UserStore.setLoggedInUser(user) on the client-side is lost, since a new instance of UserStore was created when we did new Flux()? |
@terranmoccasin you need to arrange for the browser to send the authentication token to the server on every request. If you store your user in a cookie, then this is automatic, though less secure. If you store your user in localStorage, then you need to send it in an HTTP header, which you can really only arrange for ajax calls. That means if a user Refreshes, they would need to authenticate again since the Authentication header would not be sent. |
@bman654 Ah okay, I was hoping there was a more elegant solution for using localStorage instead of cookies. For localStorage, what you're saying is that since you cant send an HTTP header on just a regular browser GET request when the user first loads the page, you need to have some AJAX call that happens after the page loads to authenticate to the server-side, and THEN render the root react app and pass it along to the client? |
Yes. Brandon
|
Another option is to send them to a page which just exists to extract the token for local storage, creates a very short term (like 1 minute) auth cookie, then redirect them to your real app, and the server can then use the cookie. The short life of the cookie makes it fairly unlikely to be stolen. Brandon
|
I've been trying to make an authentication mixin that can access the state in one of my flux stores but am not using singletons that I can make available to the static methods. Would it be alright to pass a context object through to the static willTransition hook methods to allow them access to arbitrary services you might need?