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

Synchronous onBefore result handling #3091

Closed
csvn opened this issue Oct 17, 2016 · 3 comments
Closed

Synchronous onBefore result handling #3091

csvn opened this issue Oct 17, 2016 · 3 comments
Milestone

Comments

@csvn
Copy link
Contributor

csvn commented Oct 17, 2016

Hello,

I am trying to understand how the onBefore hooks act when synchronous. The documentation says:

If any hook modifies the transition synchronously (by throwing, returning false, or returning a TargetState), the remainder of the hooks are skipped.

To me, this means that if a hook returns a modification (throw, false, targetState), the remainder of the hooks will not run at all. But it seems that currently, the hooks will run, but their result is ignored. See plunkr (logs to console).

Have I misunderstood something? Is this as intended, or is it possible to change this so only the hooks that needs to run, will be run? Because currently, one onBefore hook can run multiple times unnecessarily. Lets say we have 3 onBefore hooks (A, B, C), each modifying the transition by returning new targetState:

Currently

  1. A (new target) => B => C
  2. A => B (new target) => C
  3. A => B => C (new target)

Proposed

  1. A (new target)
  2. A => B (new target)
  3. A => B => C (new target)

I am trying to do something similar to the above example with my Authorization hook, Organization hook (redirects if param is null) and a hook which needs to be async. I don't want the last async hook to run if not needed (i.e. Auth or Org hooks already handle it).

@christopherthielen
Copy link
Contributor

christopherthielen commented Oct 20, 2016

Thanks for the great bug report and plunker!

But it seems that currently, the hooks will run, but their result is ignored. Have I misunderstood something? Is this as intended,

@csvn you haven't misunderstood, and no it's not as intended. Thanks for the bug report. If you are interested in fixing it, the relevant code is here:

https://github.com/ui-router/core/blob/master/src/transition/transitionHook.ts#L104-L120

@csvn
Copy link
Contributor Author

csvn commented Oct 21, 2016

@christopherthielen I just created a pull request for this. :)

@csvn
Copy link
Contributor Author

csvn commented Nov 3, 2016

Closing, this has been fixed by ui-router/core@8a45d04

@csvn csvn closed this as completed Nov 3, 2016
@christopherthielen christopherthielen modified the milestones: 1.0.0-beta.4, 1.0.0-final Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants