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.

Sign up for GitHub

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

Jump to bottom

fix(core): prevent BrowserModule providers from being loaded twice #45826

Closed
wants to merge 1 commit into from
Closed

fix(core): prevent BrowserModule providers from being loaded twice #45826

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

This commit updates the logic of the BrowserModule to detect a situation when it's used in the bootstrapApplication case, which already includes BrowserModule providers.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added state: WIP area: core Issues related to the framework runtime target: major This PR is targeted for the next major release cross-cutting: standalone Issues related to the NgModule-less world labels Apr 30, 2022
@ngbot ngbot bot modified the milestone: Backlog Apr 30, 2022
@AndrewKushnir AndrewKushnir force-pushed the browser-module-providers-check branch 2 times, most recently from 89b1ab3 to 278bd51 Compare April 30, 2022 05:44
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Apr 30, 2022
@AndrewKushnir AndrewKushnir marked this pull request as ready for review April 30, 2022 05:45
@pullapprove pullapprove bot requested review from alxhub, dylhunn and jessicajaniuk and removed request for pkozlowski-opensource April 30, 2022 05:45
@AndrewKushnir
Copy link
Contributor Author

Presubmit (TGP).

@AndrewKushnir AndrewKushnir force-pushed the browser-module-providers-check branch 4 times, most recently from b1f5e26 to 29cafa3 Compare May 3, 2022 00:39
@AndrewKushnir
fix(core): prevent BrowserModule providers from being loaded twice
869f7f0 
This commit updates the logic of the `BrowserModule` to detect a situation when it's used in the `bootstrapApplication` case, which already includes `BrowserModule` providers.
@AndrewKushnir AndrewKushnir force-pushed the browser-module-providers-check branch from 822315f to 869f7f0 Compare May 3, 2022 01:32
@AndrewKushnir
Copy link
Contributor Author

Presubmit (TGP) #2.

pkozlowski-opensource
pkozlowski-opensource reviewed May 3, 2022
View reviewed changes
packages/platform-browser/test/browser/bootstrap_standalone_spec.ts
@@ -21,7 +21,7 @@ describe('bootstrapApplication for standalone components', () => {

afterEach(() => {
destroyPlatform();
rootEl.remove();
rootEl?.remove();
Copy link
Member

Choose a reason for hiding this comment

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

nit / question not related to this PR: I'm curious why do we see this change?

pkozlowski-opensource
pkozlowski-opensource approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM, thnx so much for addressing all the review feedback!

dylhunn
dylhunn approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

pkozlowski-opensource
pkozlowski-opensource approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

atscott
atscott approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 3, 2022
@dylhunn
Copy link
Contributor

dylhunn commented May 3, 2022

This PR was merged into the repository by commit fa755b2.

@dylhunn dylhunn closed this in fa755b2 May 3, 2022
@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 Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfaproject alfaproject alfaproject left review comments

@dylhunn dylhunn dylhunn approved these changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@atscott atscott atscott approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cross-cutting: standalone Issues related to the NgModule-less world target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v14-candidates
Development

Successfully merging this pull request may close these issues.
5 participants
@AndrewKushnir @dylhunn @alfaproject @atscott @pkozlowski-opensource