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

Race condition on redirect 'Cannot read property 'routes' of undefined' #73

Open
Komock opened this issue Jan 24, 2021 · 4 comments
Open

Comments

@Komock
Copy link
Contributor

Komock commented Jan 24, 2021

Hi and thanks for the library.
But I have an issue 😬

Description

For example, we have a categories page (lazy module) and when we click on "Categories" link in navigation we want to redirect to default category "Books" (lazy module).
How navigation looks:
Home
— Categories
— Books

The intersection observer emits 3 links sequentially and passes them to requestIdleCallback.

  1. Router skips first link '/' because we don't have loadChildren property in the router config
  2. Router starts loading of a module associated with '/categories' link
  3. Strategy doesn't wait until '/categories' loaded and removes '/categories/books' from the list of trees (this.queue.remove(...))

So after CategoriesModule loaded strategy doesn't load BooksModule .
Last but not least if we go to page 'categories/books' and manually reload the page (cmd + r) we may get the error 'Cannot read property routes of undefined'. The error is not permanent and more often appears in FireFox and always appears at a slow network (you can get it if setup throttling to 4G in network tab).

Minimal Reproduction

Repo - https://github.com/Komock/quick-link-test
Demo - https://komock.github.io/quick-link-test/

@mgechev
Copy link
Owner

mgechev commented Jan 25, 2021

Thank you for the report, the reproduction, and the PR. All this is super valuable.

Looking at the requestIdleCallback, it definitely doesn't make sense to remove the urlTree from the queue. What do you think of simplifying the fix by removing the call this.queue.remove(routerLink.urlTree); from the link-handler.service.ts, deleting the remove method, and renaming the property from queue to registry?

The memory consumption should be relatively insignificant even for large apps with hundreds of routes and this will better reflect the responsibility of the PrefetchRegistry.

@Komock
Copy link
Contributor Author

Komock commented Jan 30, 2021

I fixed it the way you mentioned. So now all modules are loaded 🎉

But this fix doesn't solve the error "Cannot read property 'routes' of undefined" 😒
It's probably a problem in Angular Router or RouterPreloader. And I think it should be a separate issue 🤔
So now the only way to get rid of the error is to set preload: false for routes where we have redirectTo and lazy loading...

@mgechev
Copy link
Owner

mgechev commented Mar 26, 2021

Looks like we did not follow up on this one. Is there anything we can do to move forward with the fix?

@Komock
Copy link
Contributor Author

Komock commented Mar 30, 2021

I don't think it's possible to solve this bug from strategy.
The error is from class Recognizer .
Here
route._loadedConfig!.routes; - lazy module config not loaded at this moment 🤷🏻‍♂️
So I think if we yield this call until the lazy module loaded it may fix the issue. Anyway it should be PR to Angular router with more deep investigation.

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

No branches or pull requests

2 participants