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

Fork sagas and cancel them on location change #7

Merged
merged 4 commits into from
Jan 26, 2017

Conversation

victorchabbert
Copy link
Contributor

Hey ! This PR follows our discussion in #6.

I tried to handle saga cancellation without adding a new state.
The downside is that I have not figured out a way to test saga cancellation.

@jfairbank
Copy link
Owner

jfairbank commented Jan 25, 2017

Great job! I think everything looks good, and I wouldn't worry too much about testing cancelling sagas. That's still a feature I need to better integrate into redux-saga-test-plan.

There is one caveat to this new behavior that I just realized after testing the code out. If a route saga happens to synchronously throw an error, then it can bring down the whole router saga. The throw handler of the router saga can't handle forked saga errors due to the asynchronous nature of forked sagas. I believe we're going to need to use spawn instead. It's important that users handle errors in their route saga, but I don't want errors to crash the router saga. Because spawned sagas are detached, any errors they have won't affect the parent saga, which is the router saga.

Do you mind changing this to a spawn instead?

@victorchabbert
Copy link
Contributor Author

Should be good now !

I'll work on some documentation later on.

@jfairbank
Copy link
Owner

Thanks! I'll leave this unmerged for now if you plan to add on documentation. I would gladly appreciate it :) No pressure to do so, though.

@victorchabbert
Copy link
Contributor Author

No worries, let me know if I have to change anything!

@jfairbank jfairbank merged commit 1261462 into jfairbank:master Jan 26, 2017
@jfairbank
Copy link
Owner

Looks good! Thanks!

@jfairbank
Copy link
Owner

FYI, out in v2.0.0 on npm now.

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