Skip to content

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jan 5, 2015

Supersedes #606, that's the only missing piece. Let me know if you want tests for this; I'm not sure where to put them because there are not tests for addRoutes either. But this has been working for me for a few weeks so I don't think it's problematic.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 5, 2015

Don't know what's failing Travis but locally tests run fine.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 7, 2015

Note: I slightly tweaked this PR to include force parameter in dispatch.
This is necessary to force dispatch in case replaceRoutes affected current route.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 7, 2015

This still doesn't quite work as desired.

We talked with @mjackson about this and maybe it's worth extracting either some parts of dispatch or run into some kind of refresh method.

The scenario I need to support is following:

  1. User goes to /a, A matches
  2. I call replaceRoutes with new route config where /a maps to B
  3. Router should be smart enough to figure out route config changed and dispatch a change

Right now, dispatch exits early with prevPath === path.

In this PR, I tried to fix this with force parameter, but it will only kick off dispatch if I call stop() and then run() after replacing routes. This is not correct either because we want to dispatch immediately after replaceRoutes.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 7, 2015

Changed the code to add refresh() method that works correctly regardless of whether the router is running or not. I removed callback argument from dispatch because it was always the same dispatchHandler. I already needed to move dispatchHandler outside run to implement refresh. Now that dispatchHandler was at the top, passing it as the last argument in each dispatch call seemed redundant. This also removes some of the perceived cognitive load IMO (dispatch always called the same callback but it wasn't apparent until you looked closely at the source).

@yaru22
Copy link

yaru22 commented Jan 10, 2015

+1. This seems to be what I'm looking for. Can't wait to try it out.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 14, 2015

Rebased on top of current master. Can we please get this in?

@mjackson
Copy link
Member

@gaearon This looks great. Thank you :)

mjackson added a commit that referenced this pull request Jan 15, 2015
[added] router.replaceRoutes(children)
@mjackson mjackson merged commit fa405e5 into remix-run:master Jan 15, 2015
@gaearon gaearon deleted the replace-routes branch January 15, 2015 16:47
@gaearon
Copy link
Contributor Author

gaearon commented Jan 15, 2015

YEAAH

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

3 participants