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($thunks): Skip onAfterChange when route thunks dispatch a redirect #101

Merged
merged 4 commits into from
Sep 26, 2017

Conversation

sdougbrown
Copy link
Contributor

onAfterChange is erroneously called when a routethunk dispatches a redirect action - this can
cause some undesired logic to executue. This detects redirect actions in the executed routeThunks, and appropriately skips onAfterChange in that event.

#96

`onAfterChange` is erroneously called when a routethunk dispatches a redirect action - this can
cause some undesired logic to executue.  This detects redirect actions in the executed routeThunks,
and appropriately skips `onAfterChange` in that event.

faceyspacey#96
@faceyspacey faceyspacey changed the base branch from master to rudy September 26, 2017 07:37
@faceyspacey faceyspacey merged commit 2dff758 into faceyspacey:rudy Sep 26, 2017
@faceyspacey
Copy link
Owner

faceyspacey commented Sep 26, 2017

ok, so this is finally done. i've finally decided on a strategy. it's basically what i said in the other Issue where we'll do absolutely everything :)

that means this:

  • onBeforeChange now can delay route changes if a promise is returned (and possibly make a redirect before the promise resolves--good for various auth setups)
  • routes also get onBeforeChange + onAfterChange callbacks
  • there is now a global thunk option
  • they are all handled identically by the middleware and SSR capabilities
  • and as essentially a precaution we're doing what you built here so if people do synchronous redirects in thunks, things like onAfterChange that follow are not erroneously called. The recommended way is to do such redirects in onBeforeChange (especially now/soon that it will support return promises), but people may accidentally do it in a thunk, or perhaps they need the resolved state from the route change to make the full decision. In short, it's a best of all world's approach that will likely serve the most users/use-cases :)

Of the the above listed things implemented, only your PR is implemented. I'm working on the rest now. Extra special thanks for this! It's a major catalyst to finalizing this.

Otherwise, I changed the base branch of this PR to @rudy. So that's where it will live and has been merged to for now.

SIDE NOTE: it's interesting that Flow allowed for your isRedirectAction. In the past Flow wasn't smart enough to detect which keys on objects were available if you tucked away the checking into a function. You literally had to do it at the implementation of the code within the if block.

@xMartin
Copy link

xMartin commented Mar 30, 2018

Great decision! This is exactly what I'd expect as a user.

Is there a roadmap to have this available in a release?

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.

3 participants