Skip to content

Remove RouteStore #335

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

Merged
merged 15 commits into from
Oct 4, 2014
Merged

Remove RouteStore #335

merged 15 commits into from
Oct 4, 2014

Conversation

mjackson
Copy link
Member

This PR is a little messy (apologies) because I had to merge with @gaearon's changes that we merged into master, but it should preserve everything he did WRT scrolling behavior.

There may be a few bugs. Please run through the examples and let me know what you find!

@rpflorence checklist edit:

  • data-flow example: click on a taco, refresh, page is blank, introduced by 8b78703
  • master-detail and transitions examples: Router.transitionTo is not a function (should probably just search all examples to fix these)
  • all examples: if you refresh at a route and then use the back button it doesn't work (works fine, however, before a refresh, simple-master-detail is a good one to see this behavior)
  • partial app loading (while you're in there fix this maybe?! j/k we can worry about this later)

[added] Router.PathState for keeping track of the current URL path
[added] Router.RouteLookup for looking up routes
[added] Router.Transitions for transitioning to other routes
[added] Pluggable scroll behaviors
[changed] <Routes preserveScrollPosition> => <Routes scrollBehavior>
[removed] <Route preserveScrollPosition>
[removed] Router.transitionTo, Router.replaceWith, Router.goBack
Conflicts:
	modules/actions/LocationActions.js
	modules/components/Routes.js
	modules/locations/HistoryLocation.js
	modules/locations/MemoryLocation.js
	modules/mixins/PathListener.js
	modules/stores/PathStore.js
	modules/utils/makeHref.js
@gaearon
Copy link
Contributor

gaearon commented Sep 29, 2014

Just wondering: did you deliberately use the older version of my code or is it by mistake?

I've initially wanted to turn Locations to be action creators but then realized it's not at all neccessary, so my PR that you merged to master doesn't do it.

I also made a hotfix that fixes regression I introduced in #326 but it's not merged yet.

@gaearon
Copy link
Contributor

gaearon commented Sep 29, 2014

Ah, on a second look, I do see you've done it deliberately. Do you mind expanding why?

My reasoning was that it's easier if Locations don't know anything about dispatcher, so they're injectable without making it public. However that makes LocationActions a bit more complicated than it should have been.

Is there a safeguard for “action triggering another action” situation that I tried to fix with #332? (Example of regression: #334).

@mjackson
Copy link
Member Author

The main reason I like Location objects to dispatch actions is because I want to pass the action sender (another concept I borrowed from Cocoa) through to the scroll behaviors. This makes it easy to tell where the action is coming from so we know whether we should scroll or not.

Is there a safeguard for “action triggering another action”

AFAICT the only time this is a problem is when you're using transition.abort/redirect in a sync transition (because the flux dispatcher is sync as well). We could handle this separately in the BrowserTransitionHandling and ServerTransitionHandling objects, since they are responsible for handling aborted transitions.

@rpflorence Perfect. Check boxes will help me to nail down everything that needs to work before we merge. Thanks!

@mjackson mjackson changed the title Remove route store Remove RouteStore Sep 29, 2014
@ryanflorence
Copy link
Member

Alright, that's everything I found while auditing the examples

@ryanflorence
Copy link
Member

added checklist to subject message

@mjackson
Copy link
Member Author

Was thinking we could maybe fix #102 while we're at it.

@mjackson
Copy link
Member Author

@gaearon Ah, I see. So you think that when imitating the browser transitionTo should scrollTo(0, 0) ? That makes sense. Then anybody who uses Router.Transitions will get that behavior as well, not just <Link>s.

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2014

Yes, I think this is the case.

I'm not so sure about replace, as it has no direct browser equivalent. What do you think?

I also think that programmatic goBack should preserve scroll (so it should not matter whether Back was initiated by browser button or programmatically).

Finally, I chose to explicitly not handle SETUP to not interfere with browser's scrolling on page refresh.

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2014

@mjackson

I wanted to test this branch but I can't get it running because I can't seem to find a way to call transitionTo and makeHref outside the components. I often did that in my action creators, but is it not possible now?

I'm also wondering if problems with context are anything to worry about? Router seems to rely more on it now, so I wonder if this may cause any issues.

@mjackson mjackson force-pushed the remove-route-store branch from 8b78703 to 4a4a18a Compare October 3, 2014 04:42
@mjackson mjackson force-pushed the remove-route-store branch from 4a4a18a to 637c0ac Compare October 3, 2014 04:52
@mjackson
Copy link
Member Author

mjackson commented Oct 3, 2014

@gaearon

I often did that in my action creators, but is it not possible now?

Yes, that's correct. Since <Routes> components now act as their own self-contained route stores, it makes sense for them to be the ones that create URLs. So, for example, if I say transitionTo("home") I could have two <Routes> on the same page that both interpret that route differently for themselves and their descendants.

Also, this architecture makes it easier to contain things when rendering server-side, since there is no global object that directs traffic.

I suppose we could still offer some helpers for triggering navigation actions, but they would be 1) client-side only and 2) limited to using full URL paths instead of the typical (to, params, query) that transitionTo and replaceWith get. Either that or we could have some convention where we just use the static transition methods to pass along instructions to each <Routes> since the majority of users will only have one <Routes> on a page at a time anyway.

Thanks for your feedback about the scroll behavior BTW. I adjusted things to work as you suggested. Sorry I missed that in your original work. It should be in a usable state now.

@rpflorence I believe I resolved all the bugs you found. I've run through the examples but can't find anything amiss. Care to give them one more run thru?

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2014

This indeed looks correct now! Thanks @mjackson. I'd like to test-drive this PR on my project, and I'm actually happy with passing URLs to transitions, so if you could implement (1), I'd give it a spin!

@mjackson
Copy link
Member Author

mjackson commented Oct 4, 2014

@gaearon If you want to trigger transitions from your action creators, just keep a reference to the <Routes> component that you render and send your actions straight to it.

var routes = React.renderComponent(<Routes ... >, document.body);

routes.transitionTo('home');

@mjackson mjackson merged commit 77ba566 into master Oct 4, 2014
@ryanflorence ryanflorence deleted the remove-route-store branch October 30, 2014 14:28
@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