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

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

bobpace
Copy link

@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
Copy link
Member

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
Copy link

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
Copy link
Member

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

@ryanflorence
Copy link
Member

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
Copy link

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

@mjackson
Copy link
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".

@mjackson
Copy link
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.

@bobpace
Copy link
Author

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
Copy link
Member

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

@bobpace
Copy link
Author

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
Copy link

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
Copy link

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
Copy link

FoxxMD commented Jan 23, 2015

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

@kilometers
Copy link

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
Copy link
Author

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
Copy link

@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
Copy link
Member

@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
Copy link

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
Copy link

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

@FoxxMD
Copy link

FoxxMD commented Feb 4, 2015

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

@NourSammour
Copy link

@FoxxMD this way didn't work :(

@shyambalu-sellaco
Copy link

Same problem with fluxible, any updates on this?

@maxhoffmann
Copy link

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

@johanneslumpe
Copy link
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?

@nicolashery
Copy link

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
Copy link
Author

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

@armandabric
Copy link

👍

@johanneslumpe
Copy link
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.

@trshafer
Copy link

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
Copy link
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
Copy link

@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
Copy link
Contributor

idolize commented May 28, 2015

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

@lancetw
Copy link

lancetw commented Jun 1, 2015

+1

1 similar comment
@tsingson
Copy link

tsingson commented Jun 1, 2015

+1

@domarmstrong domarmstrong mentioned this pull request Jun 5, 2015
@okcoker
Copy link

okcoker commented Jun 5, 2015

+1

@ryancole
Copy link

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
Copy link
Member

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
Copy link
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?

@ryanflorence
Copy link
Member

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

@ryanflorence
Copy link
Member

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
Copy link

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
Copy link

@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
Copy link

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
Copy link

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
Copy link

@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
Copy link

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
Copy link

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 mentioned this pull request Oct 6, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 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.