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

Fix multiple redirect events on auth error #2960

Merged

Conversation

fxzhukov
Copy link
Contributor

@fxzhukov fxzhukov commented Mar 4, 2019

Closes #2939
Proposed solution:

Proposed solution: Use takeLatest instead of takeEvery saga effect to handle authentication errors.

@fxzhukov fxzhukov force-pushed the fix-multiple-redirect-on-auth-error branch 2 times, most recently from ac2d21b to de78b59 Compare March 4, 2019 19:16
@fxzhukov fxzhukov force-pushed the fix-multiple-redirect-on-auth-error branch from de78b59 to 7e5bd9f Compare March 4, 2019 19:17
@djhi djhi added this to the 2.8.0 milestone Mar 5, 2019
@djhi djhi requested review from fzaninotto and Kmaschta March 5, 2019 17:11
@fzaninotto
Copy link
Member

I think you should use a takeLeading instead of a takeLatest. The latter will cancel previous calls while the former blocks future calls until the saga completes.

@fxzhukov
Copy link
Contributor Author

fxzhukov commented Mar 8, 2019

@fzaninotto there is one problem with takeLeading - it was introduced only in v1.0.0, but we have v0.16.0 installed.
https://github.com/redux-saga/redux-saga/releases
Not sure if i can update package to the major version without breaking anything

@djhi
Copy link
Contributor

djhi commented Mar 8, 2019

I tried to upgrade redux-saga through yarn resolutions on a project and had no issues. It might impact other users though.

I think we should use takeLatest and note that we should migrate to takeLeading when releasing the next major version

@fzaninotto fzaninotto merged commit 3248831 into marmelab:master Mar 8, 2019
@fzaninotto
Copy link
Member

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.

None yet

4 participants