angular-ui / ui-router

The de-facto solution to flexible routing with nested views in AngularJS
http://ui-router.github.io/
MIT License
13.54k stars 3k forks source link

Subsequent onBefore hooks don't wait until previous async hooks have resolved #3562

Closed adamreisnz closed 6 years ago

adamreisnz commented 6 years ago

I would classify this as a bug, but it's probably by design. I would be curious to see if there are workarounds;

http://plnkr.co/edit/Nl19ySiDO2i7iS28amjA?p=preview

When you have two onBefore hooks, and one of them returns an async promise, the second hook gets called before the first hook has resolved.

This is creating a major headache for me where I want the second hook to depend on data that is being resolved asynchronously in the first hook. There is no way to do this as far as I can see, because the second hook is called straight after the first one and are processed in parallel from the looks of it.

Any thoughts?

OACDesigns commented 6 years ago

Is it not be possible to combine the two into a single onBefore hook? Then you can wait for your async data to be resolved and act accordingly.

adamreisnz commented 6 years ago

I did that now but it's not the ideal solution, as it's two separate modules.

You're able to create as many on Before hooks as you want, which is really nice to compartmentalise code, so having to dump all async code in one hook is a bit counterproductive.

On Fri, Dec 8, 2017, 05:47 Oisin Conolly notifications@github.com wrote:

Is it not be possible to combine the two into a single onBefore hook? Then you can wait for your async data to be resolved and act accordingly.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/angular-ui/ui-router/issues/3562#issuecomment-350025936, or mute the thread https://github.com/notifications/unsubscribe-auth/AAd8QtJlzjpDl8WSGoO3C0YjmU9zU54Uks5s-BaVgaJpZM4QjvnW .

christopherthielen commented 6 years ago

Thanks for the plunker! Your plunker is using a pre-release of ui-router for angularjs. I updated your plunker to use the latest release (1.0.11) and it works as desired http://plnkr.co/edit/ChjVd5kN2xZVwDs6AnNS?p=preview The behavior was changed in 1.0.0 final. It now invokes each onBefore hook in order until one of them returns a promise. It then waits for the promise before invoking the next onBefore hook (much like you described).

Make sure you're installing the scoped packages from npm and not angular-ui-router.

adamreisnz commented 6 years ago

These are the packages in my app that I am using which also exhibit this behaviour;

"@uirouter/angularjs": "^1.0.10",
"@uirouter/core": "^5.0.11",

This issue was still valid for a fact, because once I moved my code to one onBefore hook as described above the problem was resolved (but the code was uglier).

christopherthielen commented 6 years ago

This issue was still valid for a fact, because once I moved my code to one onBefore hook as described above the problem was resolved

I don't understand what you mean. Are you saying you can replicate the same behavior with 1.0.10? When I switch the plunker to 1.0.10 it still behaves correctly:

FIRST ON BEFORE HOOK
TIMEOUT RESOLVED
SECOND ON BEFORE HOOK
Transition #0-0: Started  -> "Transition#0( ''{} -> 'foo'{} )"
Transition #0-0: <- Success  "Transition#0( ''{} -> 'foo'{} )", final state: foo
adamreisnz commented 6 years ago

Yes, that's what I meant. I appreciate the plunkr works, but that means it might be something else/similar that caused the issue in our production app. I will investigate shortly.

adamreisnz commented 6 years ago

Well I've dug into our past commits and related issues, updated from 1.0.10 to 1.0.11 and split it back into two different hooks. For the love of me I can't make it fail or cause an issue anymore.

It must have been something else that I'm missing right now, but it doesn't seem like the order in which onBefore hooks are called is at fault, and that seems to be done correctly.

I thought it might have been when onBefore hooks have different conditions, but that also seems to be handled ok. Will let it rest now until something shows up again, thanks for looking into it.