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

RFC: Making *Locations full-fledged Action Creators #325

Closed
wants to merge 4 commits into from

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Sep 28, 2014

As discussed in #252, I would like to be able to differentiate between user- and browser-initiated events.
This allows us to avoid trying to restore scroll position when user clicks on links.

@mjackson and I thought that perhaps turning *Location objects into full-fledged action creators helps achieve that goal.
Current path state would move into PathStore which becomes less sophisticated.

In this pull request, I'd like to ask @mjackson and @rpflorence to comment on whether we proceed with this approach, seeing how I changed HistoryLocation and PathStore. (Scrolling is not implemented here, but this PR is pre-requisite for implementing proper scrolling anyway.)

This approach is more in line with Flux:

  • All actions (whether user's or browser's) go through the same flow
  • PathStore only keeps track of store instead of doing stuff

The downsides (?):

  • Custom Locations now need to know about LocationDispatcher, ActionTypes and PayloadSources
  • The action-creating parts of HistoryLocation will be duplicated in all Locations, they get more fat

I don't think these problems are that bad, but I'd like a few opinions before going ahead with this.
Thanks!

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2014

Actually I just realized I might not need to complicate *Locations at all. Instead of onChange, they might accept onPop which is triggered only when browser pop occurs. All dispatcher calls can be placed inside LocationActions this way.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2014

For the sake of keeping Locations simple, I moved firing actions back to LocationActions for now.
If we really need something more than onPop, we can do this later.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 28, 2014

Closing this for now, since there's a simple way.

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

1 participant