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

Router links in @defer blocks not having correct relative route #54864

Closed
1 task done
NachmanBerkowitz opened this issue Mar 14, 2024 · 24 comments
Closed
1 task done

Router links in @defer blocks not having correct relative route #54864

NachmanBerkowitz opened this issue Mar 14, 2024 · 24 comments
Assignees
Labels
area: core Issues related to the framework runtime area: router bug core: defer Issues related to @defer blocks. core: di P2 The issue is important to a large percentage of users, with a workaround state: has PR
Milestone

Comments

@NachmanBerkowitz
Copy link

Is this a regression?

  • Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

17.2.1

Description

A RouterLink with a route of ./ should route to the activated route. (eg. <a [routerLink]="'./'" >Link One</a>). But when it is in a defer block, and it is a sibling of a component that imports from material it routes to the app root.
So
@defer(on timer(1000)) { <has-material/> <a [routerLink]="'./'" >Link One</a> } routes to the root.

image

Reproduction

StackBlitz link: https://stackblitz.com/edit/stackblitz-starters-prh2zk
Steps to reproduce:

  1. Have a RouterLink in a defer block that has a relative route
  2. Give it a sibling that imports from Material

Expected Behavior

The router link route should be relative to the current route.

Actual Behavior

The router link route is from the root. The 'ActiveRoute' seems to be incorrect.

Environment

  • Angular: >= 17.2.4
  • CDK/Material: 17.2.2
  • Browser(s): edge, chrome
  • Operating System (e.g. Windows, macOS, Ubuntu): Windows
@crisbeto
Copy link
Member

This seems like either a framework or router issue. Material doesn't use the router at all. Transferring to the framework repo.

@crisbeto crisbeto transferred this issue from angular/components Mar 14, 2024
@alxhub alxhub added area: core Issues related to the framework runtime core: di bug core: defer Issues related to @defer blocks. labels Mar 14, 2024
@ngbot ngbot bot modified the milestone: needsTriage Mar 14, 2024
@alxhub
Copy link
Member

alxhub commented Mar 14, 2024

Likely related to the injector construction of @defer.

@JeanMeche
Copy link
Member

JeanMeche commented Mar 14, 2024

I could narrow it down a bit:

The ActivatedRoute has an undefined route when in a defer block and having a import (whatever import).

repro: https://stackblitz.com/edit/angular-repro-54864

@AndrewKushnir AndrewKushnir added the P2 The issue is important to a large percentage of users, with a workaround label Mar 15, 2024
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 15, 2024
@tiberiuzuld
Copy link

tiberiuzuld commented Mar 15, 2024

From what I tested in above stackblitz, the issue was introduced in v17.2.3 in this PR #52881
For us this issue blocks updating angular.

Later edit:
From my tests changing in package/core/src/defer/instructions.ts:541 at this line https://github.com/angular/angular/pull/52881/files#diff-482e2c11ba4f4436bf8138f50b7ae5927cee883230d874f0b94e475270287abcR541 from parentEnvInjector to parentInjector fixes the issue.

@AndrewKushnir
Copy link
Contributor

@NachmanBerkowitz thanks for reporting the issue.

We found the origin of the problem and we are working on a fix. There are a couple first PRs created with the necessary refactoring (#54903 and #54907) and we'll need create a couple more PRs that would fix the issue. There is no exact ETA at this moment, but the final fix would likely be available in one of the 17.3.x releases.

@AndrewKushnir AndrewKushnir changed the title Router Links in defer blocks not having correct relative route when it is a sibling to a component that imports from Angular Material Router links in @defer blocks not having correct relative route Mar 16, 2024
@AndrewKushnir AndrewKushnir self-assigned this Mar 16, 2024
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Mar 18, 2024
…` class

Currently, the `RouterOutlet` class uses a custom injector implementation to provide route-specific tokens (such as `ActivatedRoute`). This commit switches Router logic to use real `EnvironmentInjector` instead.

This commit partially addresses issue angular#54864 and there will be one more PR with extra changes to defer logic.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Mar 18, 2024
…` class

Currently, the `RouterOutlet` class uses a custom injector implementation to provide route-specific tokens (such as `ActivatedRoute`). This commit switches Router logic to use real `EnvironmentInjector` instead.

This commit partially addresses issue angular#54864 and there will be one more PR with extra changes to defer logic.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Mar 18, 2024
…` class

Currently, the `RouterOutlet` class uses a custom injector implementation to provide route-specific tokens (such as `ActivatedRoute`). This commit switches Router logic to use real `EnvironmentInjector` instead.

This commit partially addresses issue angular#54864 and there will be one more PR with extra changes to defer logic.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Mar 28, 2024
…ocks

This commit updates the `@defer` logic to establish proper injector resolution order. More specifically:

- Makes node injectors to be inspected first, similar to how it happens when `@defer` block is not used.
- Adds extra handling for the Router's `OutletInjector`, until we replace it with an `EnvironmentInjector`.

Resolves angular#54864.
Resolves angular#55028.
Resolves angular#55036.
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this issue Apr 6, 2024
…ocks (angular#55079)

This commit updates the `@defer` logic to establish proper injector resolution order. More specifically:

- Makes node injectors to be inspected first, similar to how it happens when `@defer` block is not used.
- Adds extra handling for the Router's `OutletInjector`, until we replace it with an `EnvironmentInjector`.

Resolves angular#54864.
Resolves angular#55028.
Resolves angular#55036.

PR Close angular#55079
@bboyle
Copy link

bboyle commented Apr 11, 2024

@AndrewKushnir I found this page debugging an issue today… and think I have a related issue with @defer/routerLink.

update: tested with angular v17.3.3 and v17.3.4

It's not with the active route being undefined, it's with it having a stale value.

I had a user report a broken link on a page in our app.

We have a page that contains a list of posts.
Each post has a list of attachments, each attachment uses routerLink.
The list of attachments is in a discrete component loaded with @defer (on viewport(post) where #post is set on the wrapper post element — ideally as each post enters the viewport, the attachments component is loaded (which fetches various thumbnail images)

Everything works fine when viewing a feed for the first time. The links are correct.

However, if the user navigates away to another page in the app, and then navigates to a different feed… (same component structure, but a different URL, think /feed/A vs /feed/B -- the route config is /feed/:feedID)
when the list of attachments loads (via defer again) in this new view, all the links point to the previous URL (/feed/A rather than /feed/B). I did some logging on ActivatedRoute in that component and verified it had the previous route still, which led me here.

Not sure if that's enough info to debug. I don't really know how to put together a stackblitz.
For now I've removed @defer from the app and routerLink is working correctly.

@atscott
Copy link
Contributor

atscott commented Apr 11, 2024

Not sure if that's enough info to debug.

It's not, unfortunately. A reproduction is really needed for us to investigate.

I don't really know how to put together a stackblitz.

Can you create a minimal GitHub repo instead?

@bboyle
Copy link

bboyle commented Apr 11, 2024

working on it. it's at that infuriating stage.
in a brand new repo where I'm building out a similar route structure and use of defer, no issue.
in our app, cutting out code like crazy to bring it back to the simplest form, I haven't been able to NOT reproduce it.

There is definitely a bug somewhere. I can't prove it's in angular yet, but I can't prove it's not. The fact that removing defer corrects the routerLink URLs sure smells like angular. I've disabled all guards and resolvers and ruled out those.

3+ hours, not how I want to spend my free time 😅 but the gap must be closing…

@NachmanBerkowitz
Copy link
Author

@bboyle @atscott We just rolled out with 17.3 and we are having a similar issue. Sometimes the route is 'stuck' on an old value. I am also struggling to figure out what causes that behavior and how to reproduce.

@NachmanBerkowitz
Copy link
Author

NachmanBerkowitz commented Apr 11, 2024

@bboyle @atscott I updated my stackblitz to reproduce the issue.
https://stackblitz.com/edit/stackblitz-starters-prh2zk
If you first click on 'go to test component with id 1', then click on go to 'Navigate To test 2', then click on 'Click here for test componet with id 4', then hover over the 'current route' link, it will have the wrong id in the url. It will have 1 instead of 4.

@NachmanBerkowitz
Copy link
Author

Until there is a fix we have rolled back to 17.2 (again).

@rosslavery
Copy link

We had the same issue as well. Fortunately in our case we were able to refactor some components to avoid the stale values inside the ActivatedRoute.params observable, but we observed the same behaviour. Users visiting a page, leaving, and returning to see stale data from their prior pageview (w/ different URL param).

@NachmanBerkowitz
Copy link
Author

Should we be making a new bug report for this?

@atscott atscott reopened this Apr 11, 2024
@atscott
Copy link
Contributor

atscott commented Apr 11, 2024

@AndrewKushnir is out of office until next week.

I made the reproduction a bit smaller: https://stackblitz.com/edit/stackblitz-starters-uxf7bm
I found that removing the RouterModule imports of the component in the defer block makes it work.

@bboyle
Copy link

bboyle commented Apr 11, 2024

FYI I don't have RouterModule imported anywhere in my app (standalone component migration was completed).
Will keep working on reproducing the issue.

@bboyle
Copy link

bboyle commented Apr 16, 2024

I have reproduced the issue -- here is a git repo you can run locally to see it in action: https://github.com/bboyle/angular-defer-routerlink

Follow the instructions on screen and you will get to a state where some links (which use routerLink="[]") are no longer linking to the current URL. I have used simple CSS to highlight the links, but just hover over them to compare the href to the current URL.

The issue occurs when MatTooltipModule is imported into the child component. In this example app, tooltips are not used, but in my company's app we do indeed use tooltips in the child component. Simply importing the tooltip module is enough to trigger the bug.

factors required to reproduce this bug:

let me know if you need any more details

@Yberion
Copy link

Yberion commented Apr 16, 2024

Hello, it seems that I have the same problem there, I wanted to use defer blocks to load a different layout depending on the result of the BreakpointObserver, both having a router outlet for child pages.

The problem occur with a component from Angular Material, it seems that the router and ActivatedRoute are not "synchronized" anymore.

  • https://github.com/Yberion/angular-defer
  • npm i
  • npm run start
  • Open the console (everything is logged in the console)
  • click on go to page1 from home
  • click on page2
  • click on page 1
  • click on home
  • Previous page via browser button
  • click on page2
  • click on home
  • Previous page via browser button

And it is at this point that we get different results from the router and the snapshot.

The problem appears in the component that is defered LoaderComponent, no problem in the SomepageComponent (check console logs).

The problem disappears if I remove MatButtonModule from LoaderComponent OR if I remove the defer block from SomepageComponent.

I'm on Angular 17.3.4

@NachmanBerkowitz
Copy link
Author

This bug seems like the previous bug. Using a defer block around a component that imports 'something' causes issues with the ActivatedRoute in that component.

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Apr 17, 2024
…er` blocks

`RouterOutlet` uses a unique injector logic that returns a value that correspond to the `ActivatedRoute` token dynamically. This logic breaks when a component/directive/pipe that injects the `ActivatedRoute` is located within a `@defer` block, because defer creates an `EnvironmentInjector` instance, which doesn't have that dynamic logic.

We've added some special handling of the `OutletInjector` in one of the previous commits, but it was incomplete and it was not covering cases when different routes use the same component. This commit updates defer logic to re-establish this dynamic behavior for `ActivatedRoute` by creating an instance of the `OutletInjector` when a parent injector was also an instance of `OutletInjector`.

This fix is a short-term solution and longer term we should find a way to achieve the dynamic behavior that Router relies on, but without adding a special case logic into defer.

Resolves angular#54864.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Apr 17, 2024
…er` blocks

`RouterOutlet` uses a unique injector logic that returns a value that correspond to the `ActivatedRoute` token dynamically. This logic breaks when a component/directive/pipe that injects the `ActivatedRoute` is located within a `@defer` block, because defer creates an `EnvironmentInjector` instance, which doesn't have that dynamic logic.

We've added some special handling of the `OutletInjector` in one of the previous commits, but it was incomplete and it was not covering cases when different routes use the same component. This commit updates defer logic to re-establish this dynamic behavior for `ActivatedRoute` by creating an instance of the `OutletInjector` when a parent injector was also an instance of `OutletInjector`.

This fix is a short-term solution and longer term we should find a way to achieve the dynamic behavior that Router relies on, but without adding a special case logic into defer.

Resolves angular#54864.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Apr 17, 2024
…er` blocks

`RouterOutlet` uses a unique injector logic that returns a value that correspond to the `ActivatedRoute` token dynamically. This logic breaks when a component/directive/pipe that injects the `ActivatedRoute` is located within a `@defer` block, because defer creates an `EnvironmentInjector` instance, which doesn't have that dynamic logic.

We've added some special handling of the `OutletInjector` in one of the previous commits, but it was incomplete and it was not covering cases when different routes use the same component. This commit updates defer logic to re-establish this dynamic behavior for `ActivatedRoute` by creating an instance of the `OutletInjector` when a parent injector was also an instance of `OutletInjector`.

This fix is a short-term solution and longer term we should find a way to achieve the dynamic behavior that Router relies on, but without adding a special case logic into defer.

Resolves angular#54864.
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 17, 2024

@Yberion @bboyle thanks for providing a repro. We've identified the root cause and I've created a PR with a fix: #55374. I will update this thread once the mentioned PR lands.

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Apr 17, 2024
…er` blocks

`RouterOutlet` uses a unique injector logic that returns a value that correspond to the `ActivatedRoute` token dynamically. This logic breaks when a component/directive/pipe that injects the `ActivatedRoute` is located within a `@defer` block, because defer creates an `EnvironmentInjector` instance, which doesn't have that dynamic logic.

We've added some special handling of the `OutletInjector` in one of the previous commits, but it was incomplete and it was not covering cases when different routes use the same component. This commit updates defer logic to re-establish this dynamic behavior for `ActivatedRoute` by creating an instance of the `OutletInjector` when a parent injector was also an instance of `OutletInjector`.

This fix is a short-term solution and longer term we should find a way to achieve the dynamic behavior that Router relies on, but without adding a special case logic into defer.

Resolves angular#54864.
@alxhub alxhub closed this as completed in 9894278 Apr 22, 2024
alxhub pushed a commit that referenced this issue Apr 22, 2024
…er` blocks (#55374)

`RouterOutlet` uses a unique injector logic that returns a value that correspond to the `ActivatedRoute` token dynamically. This logic breaks when a component/directive/pipe that injects the `ActivatedRoute` is located within a `@defer` block, because defer creates an `EnvironmentInjector` instance, which doesn't have that dynamic logic.

We've added some special handling of the `OutletInjector` in one of the previous commits, but it was incomplete and it was not covering cases when different routes use the same component. This commit updates defer logic to re-establish this dynamic behavior for `ActivatedRoute` by creating an instance of the `OutletInjector` when a parent injector was also an instance of `OutletInjector`.

This fix is a short-term solution and longer term we should find a way to achieve the dynamic behavior that Router relies on, but without adding a special case logic into defer.

Resolves #54864.

PR Close #55374
@AndrewKushnir
Copy link
Contributor

FYI, the fix from PR #55374 was merged and released as a part of v17.3.6. Please let us know if the problem still exists after updating to v17.3.6.

@Yberion
Copy link

Yberion commented Apr 25, 2024

Hello @AndrewKushnir, thanks a lot for the fix. I can confirm on my side that it is working now!

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime area: router bug core: defer Issues related to @defer blocks. core: di P2 The issue is important to a large percentage of users, with a workaround state: has PR
Projects
None yet