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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@bobpace

bobpace commented Dec 11, 2014

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?

    var context = {...}
    Router.create({routes: routes, transitionContext: context})

    statics: {
      willTransitionTo: function(transition) {
         var loginState = transition.context.getStore(LoginStore).getState();
      }
    }
@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Dec 15, 2014

Contributor

interesting, the basic issue is that in regular component hooks you have access to this.context but when you get into the static transition hooks you need an application container somewhere to access application things, but with this you can just specify it up-front and then we pass it in.

@mjackson thoughts?

Contributor

ryanflorence commented Dec 15, 2014

interesting, the basic issue is that in regular component hooks you have access to this.context but when you get into the static transition hooks you need an application container somewhere to access application things, but with this you can just specify it up-front and then we pass it in.

@mjackson thoughts?

@tcoopman

This comment has been minimized.

Show comment
Hide comment
@tcoopman

tcoopman Dec 19, 2014

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.

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.

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Dec 19, 2014

Contributor

Yeah, I've been thinking about this too:

  1. I dynamically create my routes here and pass in the token (or "context" stuff) with getRoutes(token), but it could very well be getRoutes(appContext) https://github.com/rackt/react-router-mega-demo/blob/master/app/client.js#L28
  2. I do a ghetto DI here with the token: https://github.com/rackt/react-router-mega-demo/blob/master/app/routes.js#L6-L8
  3. So that I can use it in the transition hook: https://github.com/rackt/react-router-mega-demo/blob/master/app/handlers/CreateContact.js#L11

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:

  1. Support supplying a "context" object for the transition hooks.
  2. Find/create a recommended application container / dependency injection strategy.

I'm leaning toward (1).

@mjackson thoughts?

Contributor

ryanflorence commented Dec 19, 2014

Yeah, I've been thinking about this too:

  1. I dynamically create my routes here and pass in the token (or "context" stuff) with getRoutes(token), but it could very well be getRoutes(appContext) https://github.com/rackt/react-router-mega-demo/blob/master/app/client.js#L28
  2. I do a ghetto DI here with the token: https://github.com/rackt/react-router-mega-demo/blob/master/app/routes.js#L6-L8
  3. So that I can use it in the transition hook: https://github.com/rackt/react-router-mega-demo/blob/master/app/handlers/CreateContact.js#L11

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:

  1. Support supplying a "context" object for the transition hooks.
  2. Find/create a recommended application container / dependency injection strategy.

I'm leaning toward (1).

@mjackson thoughts?

@mjackson

This comment has been minimized.

Show comment
Hide comment
@mjackson

mjackson Dec 19, 2014

Member

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

Member

mjackson commented Dec 19, 2014

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

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Dec 19, 2014

Contributor

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
    }
  }
});
Contributor

ryanflorence commented Dec 19, 2014

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
    }
  }
});
@tcoopman

This comment has been minimized.

Show comment
Hide comment
@tcoopman

tcoopman Dec 19, 2014

That would solve the problem indeed and looks nicer than the custom solution. Does this already work?

That would solve the problem indeed and looks nicer than the custom solution. Does this already work?

@mjackson

This comment has been minimized.

Show comment
Hide comment
@mjackson

mjackson Dec 19, 2014

Member

Ya, similar to that. You don't need the extraState prop in Router.create tho. Just stick your extra stuff on the state arg in your callback.

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

Member

mjackson commented Dec 19, 2014

Ya, similar to that. You don't need the extraState prop in Router.create tho. Just stick your extra stuff on the state arg in your callback.

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

@mjackson

This comment has been minimized.

Show comment
Hide comment
@mjackson

mjackson Jan 16, 2015

Member

@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 (transition, state) signature.

Member

mjackson commented Jan 16, 2015

@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 (transition, state) signature.

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Jan 16, 2015

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 commented Jan 16, 2015

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.

@mjackson

This comment has been minimized.

Show comment
Hide comment
@mjackson

mjackson Jan 16, 2015

Member

@bobpace so your "context" isn't state/URL specific stuff, but more on the page/request level?

Member

mjackson commented Jan 16, 2015

@bobpace so your "context" isn't state/URL specific stuff, but more on the page/request level?

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Jan 16, 2015

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.

bobpace commented Jan 16, 2015

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.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Jan 16, 2015

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.

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.

@ndreckshage

This comment has been minimized.

Show comment
Hide comment
@ndreckshage

ndreckshage Jan 23, 2015

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.

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.

@FoxxMD

This comment has been minimized.

Show comment
Hide comment
@FoxxMD

FoxxMD Jan 23, 2015

+1 , my use case is very similar to @ndreckshage

FoxxMD commented Jan 23, 2015

+1 , my use case is very similar to @ndreckshage

@kilometers

This comment has been minimized.

Show comment
Hide comment
@kilometers

kilometers Jan 23, 2015

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)

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)

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Jan 23, 2015

@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 commented Jan 23, 2015

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

@kilometers

This comment has been minimized.

Show comment
Hide comment
@kilometers

kilometers Jan 23, 2015

@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

@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

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Jan 24, 2015

Contributor

@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 getTransitionContext and pass that in as the final argument to the transition hooks, or add it to transition.

willTransitionTo (transition, params, query, context) {
  transition.context;
  // or
  context;
}
Contributor

ryanflorence commented Jan 24, 2015

@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 getTransitionContext and pass that in as the final argument to the transition hooks, or add it to transition.

willTransitionTo (transition, params, query, context) {
  transition.context;
  // or
  context;
}
@nicolashery

This comment has been minimized.

Show comment
Hide comment
@nicolashery

nicolashery Feb 2, 2015

Had the same issue using Yahoo's fluxible and react-router. I switched to @bobpace's branch and it worked out pretty well for me: nicolashery/example-isomorphic-one@810722c

I'd be ok with having a getTransitionContext() function in the options instead of transitionContext, if that is more "future-proof". Having transition.context set makes sense since we are calling it "transition context" (and we keep that context if we store the transition somewhere to retry later).

Had the same issue using Yahoo's fluxible and react-router. I switched to @bobpace's branch and it worked out pretty well for me: nicolashery/example-isomorphic-one@810722c

I'd be ok with having a getTransitionContext() function in the options instead of transitionContext, if that is more "future-proof". Having transition.context set makes sense since we are calling it "transition context" (and we keep that context if we store the transition somewhere to retry later).

@NourSammour

This comment has been minimized.

Show comment
Hide comment
@NourSammour

NourSammour Feb 4, 2015

is there any updates about this PR?
i'm trying to get floxxor work with willTransitionTo but until now unlucky

is there any updates about this PR?
i'm trying to get floxxor work with willTransitionTo but until now unlucky

@FoxxMD

This comment has been minimized.

Show comment
Hide comment
@FoxxMD

FoxxMD Feb 4, 2015

@NourSammour , you can get around it by requireing your store and accessing it directly.

FoxxMD commented Feb 4, 2015

@NourSammour , you can get around it by requireing your store and accessing it directly.

@NourSammour

This comment has been minimized.

Show comment
Hide comment
@NourSammour

NourSammour Feb 5, 2015

@FoxxMD this way didn't work :(

@FoxxMD this way didn't work :(

@shyambalu-sellaco

This comment has been minimized.

Show comment
Hide comment
@shyambalu-sellaco

shyambalu-sellaco Feb 10, 2015

Same problem with fluxible, any updates on this?

Same problem with fluxible, any updates on this?

@maxhoffmann

This comment has been minimized.

Show comment
Hide comment
@maxhoffmann

maxhoffmann Feb 13, 2015

Contributor

Would also love to see progress here. Anything I can help with?

Contributor

maxhoffmann commented Feb 13, 2015

Would also love to see progress here. Anything I can help with?

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe Feb 16, 2015

Contributor

@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?

Contributor

johanneslumpe commented Feb 16, 2015

@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?

@nicolashery

This comment has been minimized.

Show comment
Hide comment
@nicolashery

nicolashery Feb 16, 2015

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

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

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Feb 16, 2015

@nicolashery I rebased against the latest master, squashed the commits and force pushed it back up so it's current again. :)

bobpace commented Feb 16, 2015

@nicolashery I rebased against the latest master, squashed the commits and force pushed it back up so it's current again. :)

@nicolashery

This comment has been minimized.

Show comment
Hide comment
@nicolashery

nicolashery Feb 16, 2015

Thanks @bobpace! Just upgraded to your latest branch.

If anyone else was using it and wants to upgrade, make sure you remove any calls to transition.wait and use the callback instead, as it was deprecated in 0.12: https://github.com/rackt/react-router/blob/master/UPGRADE_GUIDE.md#011x---0120

Thanks @bobpace! Just upgraded to your latest branch.

If anyone else was using it and wants to upgrade, make sure you remove any calls to transition.wait and use the callback instead, as it was deprecated in 0.12: https://github.com/rackt/react-router/blob/master/UPGRADE_GUIDE.md#011x---0120

@esnunes

This comment has been minimized.

Show comment
Hide comment
@esnunes

esnunes Feb 17, 2015

I'm using it with fluxible and it works like a charm

esnunes commented Feb 17, 2015

I'm using it with fluxible and it works like a charm

@mattapperson

This comment has been minimized.

Show comment
Hide comment

+1 for this

@eiriklv

This comment has been minimized.

Show comment
Hide comment

eiriklv commented Mar 1, 2015

+1

@micahlmartin

This comment has been minimized.

Show comment
Hide comment
@micahlmartin

micahlmartin Mar 3, 2015

Contributor

I need this too. Any plans on getting it merged so we don't have to depend on a fork?

Contributor

micahlmartin commented Mar 3, 2015

I need this too. Any plans on getting it merged so we don't have to depend on a fork?

@taurose taurose referenced this pull request Mar 4, 2015

Closed

Remove RouteHandlerMixin #872

@zupzup

This comment has been minimized.

Show comment
Hide comment
@zupzup

zupzup Mar 4, 2015

Contributor

need this too, +1

Contributor

zupzup commented Mar 4, 2015

need this too, +1

@micahlmartin

This comment has been minimized.

Show comment
Hide comment
@micahlmartin

micahlmartin Mar 5, 2015

Contributor

@bobpace can you update the PR and fix the conflicts so it can get merged in? Thanks.

Contributor

micahlmartin commented Mar 5, 2015

@bobpace can you update the PR and fix the conflicts so it can get merged in? Thanks.

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe Mar 5, 2015

Contributor

Well the final API has not yet been decided upon, has it? What @ryanflorence proposed looks a bit different and also internally it is not clear, whether the context should be accessible through the Transition instance, or as a 4th parameter to the hook.

Contributor

johanneslumpe commented Mar 5, 2015

Well the final API has not yet been decided upon, has it? What @ryanflorence proposed looks a bit different and also internally it is not clear, whether the context should be accessible through the Transition instance, or as a 4th parameter to the hook.

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Mar 5, 2015

I'm happy to use any variation of this that the project owners decide is best. For my own needs I just assign it once up front on page load and so having a method that is called on each transition such as 'getTransitionContext' will always return the same value for me. If there are others who would truly want to specify something different each time (I believe this is narrowed down to client side routing only at this point, which i think whatever you could get done inside the method call you could likely address in other ways) then that's an ok outcome and I can modify this if you would like.

bobpace commented Mar 5, 2015

I'm happy to use any variation of this that the project owners decide is best. For my own needs I just assign it once up front on page load and so having a method that is called on each transition such as 'getTransitionContext' will always return the same value for me. If there are others who would truly want to specify something different each time (I believe this is narrowed down to client side routing only at this point, which i think whatever you could get done inside the method call you could likely address in other ways) then that's an ok outcome and I can modify this if you would like.

@dozoisch

This comment has been minimized.

Show comment
Hide comment
@dozoisch

dozoisch Apr 25, 2015

Contributor

👍 most wanted feature!

Contributor

dozoisch commented Apr 25, 2015

👍 most wanted feature!

@bobpace

This comment has been minimized.

Show comment
Hide comment
@bobpace

bobpace Apr 25, 2015

@sebas5384 one option would be to dispatch the state from within the router callback to a locally defined route store which would then be accessible via the context you already handed to the transition (assuming you are using something like fluxible app where the store instances are already available through the context)

bobpace commented Apr 25, 2015

@sebas5384 one option would be to dispatch the state from within the router callback to a locally defined route store which would then be accessible via the context you already handed to the transition (assuming you are using something like fluxible app where the store instances are already available through the context)

@trshafer

This comment has been minimized.

Show comment
Hide comment
@trshafer

trshafer Apr 25, 2015

@sebas5384 exactly what @bobpace mentioned. The context is meant to represent a singleton with access to all of your resources.

Let's say you have a react component that handles flash messages. If you wish to persist your flash messages over routing, it's best to use a FlashMessageStore singleton which would be available in the context. If you wish to have your flash disappear on route transitions, then you could persist the state locally in your FlashMessageComponent, or have your FlashMessageStore clear the data on a CHANGE_ROUTE action (my preferred solution). This way your entire application state is either accessible via the context, or is not meant to persist.

@sebas5384 exactly what @bobpace mentioned. The context is meant to represent a singleton with access to all of your resources.

Let's say you have a react component that handles flash messages. If you wish to persist your flash messages over routing, it's best to use a FlashMessageStore singleton which would be available in the context. If you wish to have your flash disappear on route transitions, then you could persist the state locally in your FlashMessageComponent, or have your FlashMessageStore clear the data on a CHANGE_ROUTE action (my preferred solution). This way your entire application state is either accessible via the context, or is not meant to persist.

@mathieumg

This comment has been minimized.

Show comment
Hide comment

👍 +1

@sebas5384

This comment has been minimized.

Show comment
Hide comment
@sebas5384

sebas5384 Apr 26, 2015

@bobeagan, thanks! your branch solved my needs, thanks 👍
Still, it feels that the approach of an extraState or using the actual router's state is more elegant.

@growlsworth, thanks for the tip.

@bobeagan, thanks! your branch solved my needs, thanks 👍
Still, it feels that the approach of an extraState or using the actual router's state is more elegant.

@growlsworth, thanks for the tip.

@alexfedoseev

This comment has been minimized.

Show comment
Hide comment

👍 +1

@yale

This comment has been minimized.

Show comment
Hide comment
@yale

yale May 4, 2015

👍 +1

yale commented May 4, 2015

👍 +1

@trshafer

This comment has been minimized.

Show comment
Hide comment
@trshafer

trshafer May 5, 2015

Hey @ryanflorence or @mjackson any decision on this PR yet?

trshafer commented May 5, 2015

Hey @ryanflorence or @mjackson any decision on this PR yet?

@bmcmahen

This comment has been minimized.

Show comment
Hide comment
@bmcmahen

bmcmahen May 6, 2015

Doh, I just ran into this issue too...

bmcmahen commented May 6, 2015

Doh, I just ran into this issue too...

@Spy-Seth

This comment has been minimized.

Show comment
Hide comment

Spy-Seth commented May 7, 2015

👍

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe May 7, 2015

Contributor

For everybody asking about this: Take a look at #1158

The willTransitionTo and willTransitionFrom transition hooks have been removed.

With the new version this PR becomes obsolete.

Contributor

johanneslumpe commented May 7, 2015

For everybody asking about this: Take a look at #1158

The willTransitionTo and willTransitionFrom transition hooks have been removed.

With the new version this PR becomes obsolete.

@trshafer

This comment has been minimized.

Show comment
Hide comment
@trshafer

trshafer May 7, 2015

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.

trshafer commented May 7, 2015

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.

@idolize

This comment has been minimized.

Show comment
Hide comment
@idolize

idolize May 28, 2015

Contributor

I'm having issues integrating this branch into my Browserify build:

[Gulp Build Error] Parsing file node_modules/react-router/modules/components/RouteHandler.js: Unexpected token (48:21)

package.json:

"react-router": "bobpace/react-router#transitionContext"

Anyone else having this issue? The react-router module from npm builds fine.

Contributor

idolize commented May 28, 2015

I'm having issues integrating this branch into my Browserify build:

[Gulp Build Error] Parsing file node_modules/react-router/modules/components/RouteHandler.js: Unexpected token (48:21)

package.json:

"react-router": "bobpace/react-router#transitionContext"

Anyone else having this issue? The react-router module from npm builds fine.

@nicolashery

This comment has been minimized.

Show comment
Hide comment
@nicolashery

nicolashery May 28, 2015

@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

@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

@idolize

This comment has been minimized.

Show comment
Hide comment
@idolize

idolize May 28, 2015

Contributor

@nicolashery I thought that may be the issue. Thanks for the tip!

Contributor

idolize commented May 28, 2015

@nicolashery I thought that may be the issue. Thanks for the tip!

@lancetw

This comment has been minimized.

Show comment
Hide comment

lancetw commented Jun 1, 2015

+1

@tsingson

This comment has been minimized.

Show comment
Hide comment

tsingson commented Jun 1, 2015

+1

@domarmstrong domarmstrong referenced this pull request Jun 5, 2015

Closed

isomorphic auth #1266

@okcoker

This comment has been minimized.

Show comment
Hide comment

okcoker commented Jun 5, 2015

+1

@ryancole

This comment has been minimized.

Show comment
Hide comment
@ryancole

ryancole Jun 8, 2015

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

"browserify": {
    "transform": ["babelify"]
  }

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 npm link and then you can import as usual. It doesn't seem to work though. I'm getting errors when using it, but not when using current master branch.

ryancole commented Jun 8, 2015

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

"browserify": {
    "transform": ["babelify"]
  }

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 npm link and then you can import as usual. It doesn't seem to work though. I'm getting errors when using it, but not when using current master branch.

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Jun 15, 2015

Contributor

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:

  1. Transition hooks moved to route definitions
  2. routes end up being simple objects with whatever properties you want

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.

Contributor

ryanflorence commented Jun 15, 2015

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:

  1. Transition hooks moved to route definitions
  2. routes end up being simple objects with whatever properties you want

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.

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe Jun 15, 2015

Contributor

@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?

Contributor

johanneslumpe commented Jun 15, 2015

@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?

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Jun 18, 2015

Contributor

either recreate your routes (they're just lightweight objects) or use an application container, we aren't going to be an app container.

Contributor

ryanflorence commented Jun 18, 2015

either recreate your routes (they're just lightweight objects) or use an application container, we aren't going to be an app container.

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Jun 18, 2015

Contributor

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 createElement, whatever you'd like:

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>
  }
});
Contributor

ryanflorence commented Jun 18, 2015

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 createElement, whatever you'd like:

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>
  }
});
@bman654

This comment has been minimized.

Show comment
Hide comment
@bman654

bman654 Jun 30, 2015

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 global.appContext to your context once in your client initialization code.

bman654 commented Jun 30, 2015

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 global.appContext to your context once in your client initialization code.

@voronianski

This comment has been minimized.

Show comment
Hide comment
@voronianski

voronianski Jul 2, 2015

@ryanflorence sorry, too much different points/comments in this thread. Regarding ReactRouter version 0.13.x - what is possible workaround to have flux instance inside the hook?

@ryanflorence sorry, too much different points/comments in this thread. Regarding ReactRouter version 0.13.x - what is possible workaround to have flux instance inside the hook?

@terranmoccasin

This comment has been minimized.

Show comment
Hide comment
@terranmoccasin

terranmoccasin Jul 19, 2015

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()?

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()?

@bman654

This comment has been minimized.

Show comment
Hide comment
@bman654

bman654 Jul 19, 2015

@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 commented Jul 19, 2015

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

@terranmoccasin

This comment has been minimized.

Show comment
Hide comment
@terranmoccasin

terranmoccasin Jul 19, 2015

@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?

@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?

@bman654

This comment has been minimized.

Show comment
Hide comment
@bman654

bman654 Jul 19, 2015

Yes.

Brandon

On Jul 19, 2015, at 5:54 PM, Michael Hwang notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

bman654 commented Jul 19, 2015

Yes.

Brandon

On Jul 19, 2015, at 5:54 PM, Michael Hwang notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

@bman654

This comment has been minimized.

Show comment
Hide comment
@bman654

bman654 Jul 19, 2015

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

On Jul 19, 2015, at 5:54 PM, Michael Hwang notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

bman654 commented Jul 19, 2015

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

On Jul 19, 2015, at 5:54 PM, Michael Hwang notifications@github.com wrote:

@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?


Reply to this email directly or view it on GitHub.

@th0r th0r referenced this pull request Oct 6, 2015

Closed

Hook parameters #2044

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment