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

feat(upgrade): support NgModule class as an argument of the downgradeModule function #43973

Closed
wants to merge 3 commits into from
Closed

feat(upgrade): support NgModule class as an argument of the downgradeModule function #43973

wants to merge 3 commits into from

Conversation

AndrewKushnir
Copy link
Contributor

This commit extends the logic of the downgradeModule function to support NgModule class as an argument. This is needed to simplify the API surface to avoid the need to resolve NgModule factory before invoking the downgradeModule method.

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer area: upgrade Issues related to AngularJS → Angular upgrade APIs target: minor This PR is targeted for the next minor release labels Oct 28, 2021
@google-cla google-cla bot added the cla: yes label Oct 28, 2021
@ngbot ngbot bot modified the milestone: Backlog Oct 28, 2021
@AndrewKushnir AndrewKushnir added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 28, 2021
@AndrewKushnir AndrewKushnir marked this pull request as draft October 28, 2021 05:22
gkalpak
gkalpak reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉

packages/upgrade/src/common/src/util.ts Outdated Show resolved Hide resolved
packages/upgrade/static/src/downgrade_module.ts Outdated Show resolved Hide resolved
packages/upgrade/static/src/downgrade_module.ts Outdated Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Oct 28, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 28, 2021
@AndrewKushnir
feat(upgrade): support NgModule class as an argument of the `downgrad…
28880f9 
…eModule` function

This commit extends the logic of the `downgradeModule` function to support NgModule class as an argument. This is needed to simplify the API surface to avoid the need to resolve NgModule factory before invoking the `downgradeModule` method.
@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 29, 2021
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Oct 29, 2021 •
edited

Exploratory presubmit + global presubmit.

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 29, 2021
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Oct 29, 2021 •
edited

@petebacondarwin @gkalpak FYI I've tested the code in g3 both as is as well as by modifying apps code to use the new codepaths (by using the factory-less NgModule reference). Everything looks good, so this PR is ready for the final review. Could you please take a look when you get a chance? Thank you.

@AndrewKushnir AndrewKushnir marked this pull request as ready for review October 29, 2021 22:58
petebacondarwin
petebacondarwin approved these changes Oct 30, 2021
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Great sleuthing about that test, which only should run in Ivy.

petebacondarwin
petebacondarwin approved these changes Oct 30, 2021
View reviewed changes
Copy link
Member

@petebacondarwin petebacondarwin 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 the action: presubmit The PR is in need of a google3 presubmit label Nov 1, 2021
@AndrewKushnir
Copy link
Contributor Author

Global Presubmit #2.

jessicajaniuk
jessicajaniuk approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM 🍪

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from jelbourn November 1, 2021 16:52
@AndrewKushnir AndrewKushnir removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Nov 1, 2021
IgorMinar
IgorMinar approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Nov 1, 2021
@atscott
Copy link
Contributor

atscott commented Nov 4, 2021

This PR was merged into the repository by commit 34f9909.

@atscott atscott closed this in 34f9909 Nov 4, 2021
This was referenced Dec 1, 2021
Closed
Closed
Closed
@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 Dec 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak left review comments

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: upgrade Issues related to AngularJS → Angular upgrade APIs cla: yes feature Issue that requests a new feature target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AndrewKushnir @atscott @petebacondarwin @IgorMinar @gkalpak @jessicajaniuk