Skip to content

fix(router): error if module is destroyed before location is initialized #42560

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

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

This is something I ran into while working on a fix for the TestBed module teardown behavior for #18831. In the RouterInitializer.appInitializer we have a callback to the LOCATION_INITIALIZED which has to do some DI lookups. The problem is that if the module is destroyed before the location promise resolves, the Injector.get calls will fail. This is unlikely to happen in a real app, but it'll show up in unit tests once the test module teardown behavior is fixed.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: router target: patch This PR is targeted for the next patch release labels Jun 12, 2021
@ngbot ngbot bot added this to the Backlog milestone Jun 12, 2021
@google-cla google-cla bot added the cla: yes label Jun 12, 2021
if (this.destroyed) {
return Promise.resolve(true);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way of solving this would be to pull the injector.get calls outside of the callback. I decided against it, because I wasn't sure if the Router and RouterConfiguration would be available earlier in the lifecycle.

This is something I ran into while working on a fix for the `TestBed` module teardown behavior for angular#18831. In the `RouterInitializer.appInitializer` we have a callback to the `LOCATION_INITIALIZED` which has to do some DI lookups. The problem is that if the module is destroyed before the location promise resolves, the `Injector.get` calls will fail. This is unlikely to happen in a real app, but it'll show up in unit tests once the test module teardown behavior is fixed.
@crisbeto crisbeto force-pushed the router-init-error branch from 535689d to 853d031 Compare June 12, 2021 06:36
@crisbeto crisbeto marked this pull request as ready for review June 12, 2021 06:54
@pullapprove pullapprove bot requested a review from atscott June 12, 2021 06:54
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 14, 2021
@atscott
Copy link
Contributor

atscott commented Jun 14, 2021

presubmit

dylhunn pushed a commit that referenced this pull request Jun 17, 2021
…zed (#42560)

This is something I ran into while working on a fix for the `TestBed` module teardown behavior for #18831. In the `RouterInitializer.appInitializer` we have a callback to the `LOCATION_INITIALIZED` which has to do some DI lookups. The problem is that if the module is destroyed before the location promise resolves, the `Injector.get` calls will fail. This is unlikely to happen in a real app, but it'll show up in unit tests once the test module teardown behavior is fixed.

PR Close #42560
@dylhunn dylhunn closed this in 07c1ddc Jun 17, 2021
@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 Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: router cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants