RFC: router.stop() and router.setOptions()

[gaearon]

No description provided.

[gaearon]

To clarify, this is my use case:

'use strict';

var routes = require('./routes'),
    Router = require('react-router');

var router = Router.create({
  routes: routes,
  location: Router.HistoryLocation
});

function render(Handler, state) {
  // ...
}

router.run(render);

// this is webpack's hot module reloading:
module.hot.accept('./routes', function () {
  routes = require('./routes'); // obtain new version of the module

  router.stop();
  router.setOptions({ routes: routes });
  router.run(render);
});

Probably, a better API would be to have router.setOptions method. Router.create would just use it under the hood, and anyone who wants to change options later (like me) can do that. This would spare us weird replaceRoutes method.

Also pause should probably be called stop.

Thoughts?

[gaearon]

I added stop() and setOptions() and I think it fits in pretty well and doesn't look outlandish.

[guillaume86]

+1 for this, I have a very similar use case.
Just being able to change the routes config would also work for me but the .run() need a .stop() counterpart anyway.

[ryanflorence]

I think I like this. A little busy at the moment to spend a lot of thought on it personally, but please keep this conversation going :)

[gaearon]

OK, I'll do my best to add some tests later this week!

[camshaft]

+1. this is definitely needed for hot reloading after the API changes

[gaearon]

I don't think I'm going to have the time this week to write the tests, and later I'm on holiday.
If somebody volunteers to pick it up, I'm all for it!

[mjackson]

@gaearon I like stop and replaceRoutes (we already have addRoutes. just use that).

Also, let's fix the weirdness that was introduced in 6417285 along the way.

[gaearon]

@mjackson Do you think replaceRoutes > setOptions? I think setOptions is cleaner because it isn't specific and can be used by Router.create too.

[mjackson]

@gaearon I've tried as hard as I can thus far to avoid passing an "options" hash to Router.create. Options hashes are messy IMO, and the router's scope is small enough that we should be able to avoid it. Also, once you introduce an options hash it sort of becomes the default place to add everything, which leads to poor API design later on.

Anyway, just my opinion obviously. Feel free to disagree :)

[gaearon]

I see your point now! Let's proceed with replaceRoutes then and take another look if any use case for changing other options midflight comes up.

[mjackson]

@gaearon I implemented router.stop in 75c6206, which is half of the work needed here. If you'd like to do replaceRoutes in this PR, that would be great. Otherwise we can close and do it later.

[gaearon]

Let's do it later! I'm on a vacation now and can do replaceRoutes separately later if no one beats me to it.

[mjackson]

@gaearon Sounds good. Enjoy your holiday! :)