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

feature/before-route-change #12

Merged
merged 4 commits into from
Apr 26, 2017

Conversation

TeoTN
Copy link
Contributor

@TeoTN TeoTN commented Apr 1, 2017

Implement #8 - optional parallel call to a saga before any other sagas

Copy link
Owner

@jfairbank jfairbank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Only issue is that we need to ensure that beforeRouteChange runs before the matched route saga.

src/router.js Outdated
@@ -56,6 +56,10 @@ export default function router(history, routes) {
if (match) {
lastMatch = match;
effects.push(spawn(match.action, match.params));

if ('beforeRouteChange' in options) {
effects.unshift(spawn(options.beforeRouteChange, match.params));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By placing this in the array, it will potentially run in parallel to the matched route instead of technically running before it. We'll need to ensure that it can run first. If you need any help with this, let me know.

@TeoTN TeoTN force-pushed the feature/before-route-change branch from 76f0062 to 15460d0 Compare April 15, 2017 18:53
@TeoTN
Copy link
Contributor Author

TeoTN commented Apr 15, 2017

I have reimplemented it in a synchronous manner by adding an optional extra state. I hope that you'll like the solution :)
BTW I really enjoyed working with your code, it's extra clean and extensible.

src/router.js Outdated
next: HANDLE_LOCATION,
};
},

[HANDLE_LOCATION](location, fsm) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we hit this line after BEFORE_HANDLE_LOCATION, we will have lost reference to the location. In fact, the location argument will be equal to a task object that redux-saga will return for the spawn effect. We'll need to probably save a reference to the location. Maybe create a let currentLocation with the other let declarations above? Then HANDLE_LOCATION will have to use the currentLocation if the location argument is missing.

We'll also need another state after BEFORE_HANDLE_LOCATION. While the spawn is outside the array now, it is still a non-blocking effect in redux-saga, meaning it has the potential to run concurrently with the matched saga. If you add an additional state, you can use the join effect on the task to basically wait for the beforeRouteChange saga to finish before handling the location with the route saga.

@jfairbank
Copy link
Owner

This is a good start @TeoTN!

I left some more detail in my review comment. Please let me know if my comment makes sense. Also, working with fsm-iterator can be cumbersome, so let me know if you have any issues with the implementation. Thanks!

@TeoTN TeoTN force-pushed the feature/before-route-change branch from 6649fa0 to d62f102 Compare April 20, 2017 22:32
@TeoTN
Copy link
Contributor Author

TeoTN commented Apr 20, 2017

Thanks for your review @jfairbank! You've pointed out a few quite important things that I had overlooked.
I hope that my latest commit solves the problem, although I'm not sure whether I'm testing it properly, i.e. by passing task mock via next argument.
The fsm-iterator itself seems to be rather intuitive, at least to the extent I use it and after having seen my mistakes with the way I use it :)
I'll be waiting for the next review impatiently, thanks!

@jfairbank jfairbank merged commit 8bca7a4 into jfairbank:master Apr 26, 2017
@jfairbank
Copy link
Owner

This looks good, @TeoTN. Good job!

I'll work on a release soon. Thanks!

@jfairbank jfairbank mentioned this pull request Apr 26, 2017
@TeoTN
Copy link
Contributor Author

TeoTN commented Apr 28, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants