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

route-href binding too late #46

Closed
EisenbergEffect opened this Issue Nov 29, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@EisenbergEffect
Member

EisenbergEffect commented Nov 29, 2016

@fkleuver commented on Mon Nov 28 2016

I'm submitting a bug report

  • Library Version:
    1.0.7

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    6.9.1

  • NPM Version:
    4.0.2
  • Aurelia-CLI
    0.22.0

  • Browser:
    all

  • Language:
    TypeScript 2.0.10

Current behavior:
Given an application with a recursive route 'app' (but it seems to apply to nested child routes in general) where navigation occurs through a tags with route-href attributes, the following happens:

  • Click on the first route and it navigates to '#/app'
  • Click on the second route and it navigates to '#/app/app'
  • Click on the third route and it navigates to '#/app/app/app'

Then after a refresh:

  • Click on the third route and it navigates to '#/app'

Expected/desired behavior:
After the refresh:

  • Click on the third route and it navigates to '#/app/app/app'

repro
This is from a question in gitter.
The person who asked about this (@cabarney) pinpointed it down to a wrong order of sequence in which the route-href is databound when refreshing:

  1. _refreshBaseUrl on the first level route
  2. _refreshBaseUrl on the second level route
  3. generate on the router for the third level route
  4. _refreshBaseUrl on the third level route

So I tried to force step 3. to come after step 4. by moving this.processChange(); from bind() to attached() in route-href.js and that seems to solve this particular problem.

I've made a minimal repro here:
https://github.com/fkleuver/aurelia-route-href-issue

You can see the difference by clicking App several times, refreshing, then clicking the last App again. The normal route-href will fail, but the route-href-attached will work.

I don't know if moving this.processChange() to attached will break anything else though.

EDIT
Sorry, this issue was meant to go to templating-router. Is it possible to move it?

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Nov 29, 2016

Member

@fkleuver I don't think there are any really issues from moving it to attached. I would be interested to find out why this is necessary. I'm wondering if there's something wrong elsewhere that is causing this. Are you interested in investigating further? If not, would you at least be willing to submit a PR for the timing change in the route-href attribute?

Member

EisenbergEffect commented Nov 29, 2016

@fkleuver I don't think there are any really issues from moving it to attached. I would be interested to find out why this is necessary. I'm wondering if there's something wrong elsewhere that is causing this. Are you interested in investigating further? If not, would you at least be willing to submit a PR for the timing change in the route-href attribute?

@fkleuver

This comment has been minimized.

Show comment
Hide comment
@fkleuver

fkleuver Nov 29, 2016

Member

@EisenbergEffect Interesting.. I think I found the problem:

CommitChangesStep comes after ActivateNextStep in the router pipeline. CommitChangesStep
refreshes the baseUrl on the router, among other things. This baseUrl is needed for route-href to get the correct relative url.

Here's where it goes wrong: ActivateNextStep kicks off the ViewModel lifecycle without actually waiting for the rest of the router pipeline to complete. So views are already being created and binded while CommitChangesStep is still doing stuff. This means there is no guarantee that baseUrl (and potentially other properties) are complete/correct by the time the ViewModel lifecycle starts.

This is generally not a problem because with one-by-one navigation the router is always "one ahead" of the viewmodel. However, when directly navigating to the full url the nested routes are all loaded at once and then this race condition pops up. It happens consistently for all child routes which are nested more than 2 levels deep.

Furthermore, simply wrapping this.processChange() in a queueMicroTask instead of placing it inside attached() also solves the problem. This makes sense imo, because it does enough to postpone this.processChange() until the router pipeline is fully done.

That being said, I don't think it's route-href that needs to be fixed but rather something in the router pipeline. But that might bring additional ramifications with it.

What's your opinion on this?

Member

fkleuver commented Nov 29, 2016

@EisenbergEffect Interesting.. I think I found the problem:

CommitChangesStep comes after ActivateNextStep in the router pipeline. CommitChangesStep
refreshes the baseUrl on the router, among other things. This baseUrl is needed for route-href to get the correct relative url.

Here's where it goes wrong: ActivateNextStep kicks off the ViewModel lifecycle without actually waiting for the rest of the router pipeline to complete. So views are already being created and binded while CommitChangesStep is still doing stuff. This means there is no guarantee that baseUrl (and potentially other properties) are complete/correct by the time the ViewModel lifecycle starts.

This is generally not a problem because with one-by-one navigation the router is always "one ahead" of the viewmodel. However, when directly navigating to the full url the nested routes are all loaded at once and then this race condition pops up. It happens consistently for all child routes which are nested more than 2 levels deep.

Furthermore, simply wrapping this.processChange() in a queueMicroTask instead of placing it inside attached() also solves the problem. This makes sense imo, because it does enough to postpone this.processChange() until the router pipeline is fully done.

That being said, I don't think it's route-href that needs to be fixed but rather something in the router pipeline. But that might bring additional ramifications with it.

What's your opinion on this?

@EisenbergEffect EisenbergEffect self-assigned this Nov 30, 2016

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Nov 30, 2016

Member

Good analysis. Thanks for taking the time to look into that. In the worst case, we can delay the href set until attached. That's not a huge deal. Ideally, we want to delay route generation until right after the base url is fixed. Maybe we can introduce a hook for this. We might just use attached in the short term and then switch to another mechanism in the future. I need to consider it more...

Member

EisenbergEffect commented Nov 30, 2016

Good analysis. Thanks for taking the time to look into that. In the worst case, we can delay the href set until attached. That's not a huge deal. Ideally, we want to delay route generation until right after the base url is fixed. Maybe we can introduce a hook for this. We might just use attached in the short term and then switch to another mechanism in the future. I need to consider it more...

@fkleuver

This comment has been minimized.

Show comment
Hide comment
@fkleuver

fkleuver Nov 30, 2016

Member

Hm, that brought me on a different idea. I tried using the BindingEngine to subscribe to changes to router.baseUrl and call processChanges() when that happens. It works like a charm. Besides, wouldn't you want to update the hrefs when the router's baseUrl changes anyway?

The only two downsides I can think of are the added dependency on aurelia-binding and potentially an impact on performance. I don't know if that matters much though. If it doesn't, I can create a PR for that. Otherwise I'll stick to moving it to attached. Let me know..

Member

fkleuver commented Nov 30, 2016

Hm, that brought me on a different idea. I tried using the BindingEngine to subscribe to changes to router.baseUrl and call processChanges() when that happens. It works like a charm. Besides, wouldn't you want to update the hrefs when the router's baseUrl changes anyway?

The only two downsides I can think of are the added dependency on aurelia-binding and potentially an impact on performance. I don't know if that matters much though. If it doesn't, I can create a PR for that. Otherwise I'll stick to moving it to attached. Let me know..

@paulharker

This comment has been minimized.

Show comment
Hide comment
@paulharker

paulharker Feb 2, 2017

Hi all,
I have a heavily child-router based application that is suffering from this bug. We are in the framework phase, but within a few weeks to a month will start moving to a feature-creation phase. Would love to know how tis fix is coming. Paul

paulharker commented Feb 2, 2017

Hi all,
I have a heavily child-router based application that is suffering from this bug. We are in the framework phase, but within a few weeks to a month will start moving to a feature-creation phase. Would love to know how tis fix is coming. Paul

@bryanrsmith

This comment has been minimized.

Show comment
Hide comment
@bryanrsmith

bryanrsmith Mar 7, 2017

Member

@fkleuver @EisenbergEffect Do you guys know if this issue is blocked on a decision? I agree with @fkleuver's assessment that the router's commit step happening at the end of the lifecycle is the underlying issue. Since that's fairly involved to change at this point it would be great to work around the issue in route-href to at least get some people unblocked. What do you guys think of simply delaying the generation as Rob suggests, pending further discussions? I can help out with this if needed (but it sounds like there may already be a couple alternatives implemented already?)

Member

bryanrsmith commented Mar 7, 2017

@fkleuver @EisenbergEffect Do you guys know if this issue is blocked on a decision? I agree with @fkleuver's assessment that the router's commit step happening at the end of the lifecycle is the underlying issue. Since that's fairly involved to change at this point it would be great to work around the issue in route-href to at least get some people unblocked. What do you guys think of simply delaying the generation as Rob suggests, pending further discussions? I can help out with this if needed (but it sounds like there may already be a couple alternatives implemented already?)

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Mar 7, 2017

Member

I think we can move this forward by delaying until after attached for now. @bryanrsmith If you have some time to make the fix, that would be awesome.

Member

EisenbergEffect commented Mar 7, 2017

I think we can move this forward by delaying until after attached for now. @bryanrsmith If you have some time to make the fix, that would be awesome.

@matthewh

This comment has been minimized.

Show comment
Hide comment
@matthewh

matthewh Jun 5, 2017

What is needed to get this merged? I am using @bryanrsmith's fix and it works well.

matthewh commented Jun 5, 2017

What is needed to get this merged? I am using @bryanrsmith's fix and it works well.

@aurelia aurelia deleted a comment from shahidkarimi Nov 23, 2017

@davismj davismj assigned davismj and unassigned EisenbergEffect Nov 23, 2017

davismj added a commit to davismj/router that referenced this issue Nov 23, 2017

fix(navigation-instruction): Wait to swap child routes
The waitToSwap variable was not properly being passed to child routes' _commitChanges(), which introduced a timing issue where a child router's baseUrl was not updated before it was bound. This commit adds the variable back in.

Resolves aurelia#306
Resolves aurelia#552
Resolves aurelia/templating-router#46

davismj added a commit to davismj/templating-router that referenced this issue Nov 23, 2017

change(route-href): Generate route href on bind
There was a timing issue aurelia#46 that caused route-href to break when opening a page with child routers on first open. A temporary fix aurelia#56 was made that delayed generation of the route until the attached callback, when all requisite data was finally available. This solution remained a blocker aurelia/router#552 for SSR. I've introduced another hopefully more robust fix aurelia/router#555 that should allow us to return to the original behavior of generating a route on bind.

Resolves aurelia#46
Resolves aurelia/router#552
Resolves aurelia/router#306
Resolves aurelia/router#169
Depends on aurelia/router#555
Reverts aurelia#56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment