Skip to content

refactor(compiler): drop obsolete NgFactory and NgSummary config options#48268

Closed
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:rm-factories-shim
Closed

refactor(compiler): drop obsolete NgFactory and NgSummary config options#48268
AndrewKushnir wants to merge 1 commit intoangular:mainfrom
AndrewKushnir:rm-factories-shim

Conversation

@AndrewKushnir
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir commented Nov 29, 2022 •
edited
Loading

The options to generate NgFactory and NgSummary files were added to Ivy for backwards compatibility with ViewEngine. Since ViewEngine was deprecated and removed, the NgFactory and NgSummary files are no longer used as well.

This commit drops obsolete options to generate NgFactory and NgSummary files. Also, the logic that generates those files is removed.

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added state: WIP area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Nov 29, 2022
@ngbot ngbot bot modified the milestone: Backlog Nov 29, 2022
@AndrewKushnir AndrewKushnir force-pushed the rm-factories-shim branch 2 times, most recently from a30de0a to 1a9b098 Compare December 1, 2022 03:07
@AndrewKushnir
Copy link
Copy Markdown
Contributor Author

AndrewKushnir commented Dec 1, 2022

Presubmit.

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Dec 2, 2022
@AndrewKushnir AndrewKushnir marked this pull request as ready for review December 2, 2022 01:49
@pullapprove pullapprove bot requested a review from josephperrott December 2, 2022 01:49
dgp1130
dgp1130 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

Bazel changes LGTM.

/cc @devversion as he might be interested.

devversion
devversion approved these changes Dec 2, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for Bazel. Do you plan on doing the same internally, or are the shims still needed there? (I know ngc-wrapped is no longer synced but I still prefer to keep it somewhat in sync at this point)

@AndrewKushnir
Copy link
Copy Markdown
Contributor Author

AndrewKushnir commented Dec 2, 2022

@devversion thanks for the review! I've submitted a change today with a similar cleanup to the internal ng_module rule.

@AndrewKushnir
Copy link
Copy Markdown
Contributor Author

AndrewKushnir commented Dec 2, 2022

@devversion sorry, I've misunderstood a part of the question. For ng_module rule - similar changes have landed today. I will also do a cleanup in the ngc-wrapped internal code as well (there will be no need in extra code there after full removal of the code as a part of this PR).

@devversion
Copy link
Copy Markdown
Member

devversion commented Dec 2, 2022

Makes sense! thx for clarifying

@AndrewKushnir
refactor(compiler): drop obsolete NgFactory and NgSummary config options
e989c55 
The options to generate NgFactory and NgSummary files were added to Ivy for backwards compatibility with ViewEngine. Since ViewEngine was deprecated and removed, the NgFactory and NgSummary files are no longer used as well.

This commit drops obsolete options to generate NgFactory and NgSummary files. Also, the logic that generates those files is also removed.
@AndrewKushnir
Copy link
Copy Markdown
Contributor Author

AndrewKushnir commented Feb 19, 2023

TGP.

@AndrewKushnir AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2023
@AndrewKushnir
Copy link
Copy Markdown
Contributor Author

AndrewKushnir commented Feb 21, 2023 •
edited
Loading

Caretaker note: @alxhub FYI this PR is reviewed by a couple team members, but waiting for the fw-compiler approval. Could you please take a quick look before merging?

@AndrewKushnir AndrewKushnir removed the action: global presubmit The PR is in need of a google3 global presubmit label Feb 21, 2023
jessicajaniuk
jessicajaniuk approved these changes Feb 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

reviewed-for: fw-core, fw-common, fw-upgrade, fw-benchmarks, integration-tests, public-api

@pullapprove pullapprove bot requested a review from atscott February 21, 2023 21:01
@alxhub
Copy link
Copy Markdown
Member

alxhub commented Feb 21, 2023

This PR was merged into the repository by commit 83a6e20.

@alxhub alxhub closed this in 83a6e20 Feb 21, 2023
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Mar 24, 2023

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 Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub alxhub approved these changes

@devversion devversion devversion approved these changes

@dgp1130 dgp1130 dgp1130 approved these changes

@josephperrott josephperrott Awaiting requested review from josephperrott

@dylhunn dylhunn Awaiting requested review from dylhunn

@pkozlowski-opensource pkozlowski-opensource Awaiting requested review from pkozlowski-opensource

@atscott atscott Awaiting requested review from atscott

+1 more reviewer

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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.

5 participants

@AndrewKushnir @devversion @alxhub @dgp1130 @jessicajaniuk