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

Jump to bottom

perf(core): use ngDevMode to tree-shake checkNoChanges #39964

Closed
wants to merge 8 commits into from
Closed

perf(core): use ngDevMode to tree-shake checkNoChanges #39964

wants to merge 8 commits into from

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented Dec 3, 2020 •
edited

PR Checklist

PR Type

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • Yes
  • No

@google-cla google-cla bot added the cla: yes label Dec 3, 2020
@pullapprove pullapprove bot requested a review from alxhub December 3, 2020 23:03
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Dec 3, 2020 •
edited

Hi @arturovt, thanks for creating PRs to improve tree-shaking 👍

I'd like to propose an idea to group all commits from other PRs (except the one that is already marked for merge) into a single PR (while keeping changes in separate commits). That should help review these changes as a whole and would simplify the process of landing these changes.

Thank you.

@AndrewKushnir
Copy link
Contributor

@arturovt another benefit is that you may need to update payload sizes for test/demo apps (reducing the size), so there might be code conflicts that you would need to resolve multiple times...

@arturovt
Copy link
Contributor Author

arturovt commented Dec 4, 2020

@arturovt another benefit is that you may need to update payload sizes for test/demo apps (reducing the size), so there might be code conflicts that you would need to resolve multiple times...

Yeah that makes sense. I was just thinking that it's a bad idea to have changes, that affect different packages, in 1 PR.

@arturovt
perf(core): use ngDevMode to tree-shake checkNoChanges
951a13e 
This commit adds `ngDevMode` guard to run `checkNoChanges` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this code from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.
@arturovt
perf(common): use ngDevMode to tree-shake warnings
4c4f486 
This commit adds ngDevMode guard to show warnings only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake these warnings from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.
@arturovt
perf(animations): use ngDevMode to tree-shake warning
f0c7f5d 
This commit adds ngDevMode guard to show warning only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake this warning from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.
@pullapprove pullapprove bot requested a review from IgorMinar December 4, 2020 11:00
@arturovt
perf(forms): use ngDevMode to tree-shake _ngModelWarning
d2df865 
This commit adds `ngDevMode` guard to call `_ngModelWarning` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this function from production builds
(since it will act as no-op, in dev mode everything will work as it works right now)
to decrease production bundle size.
AndrewKushnir
AndrewKushnir requested changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for grouping commits into a single PR @arturovt 👍

The changes look good, just left a comment were I believe we'd still need to use isDevMode() check. Could you please have a look when you get a chance?

packages/core/src/application_ref.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime area: forms area: performance target: patch This PR is targeted for the next patch release labels Dec 4, 2020
@ngbot ngbot bot modified the milestone: Backlog Dec 4, 2020
@mhevery mhevery added this to Needs review in mhevery-review-queue via automation Dec 4, 2020
@mhevery mhevery moved this from Needs review to CleanUp in mhevery-review-queue Dec 4, 2020
AndrewKushnir
AndrewKushnir approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback @arturovt 👍

mhevery-review-queue automation moved this from CleanUp to Waiting for author Dec 4, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added 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 Dec 4, 2020
mhevery
mhevery approved these changes Dec 5, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery 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: global-approvers

mhevery-review-queue automation moved this from CleanUp to Waiting for author Dec 5, 2020
@mhevery mhevery removed the action: presubmit The PR is in need of a google3 presubmit label Dec 5, 2020
@mhevery mhevery moved this from Waiting for author to Done in mhevery-review-queue Dec 5, 2020
@mhevery mhevery added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Dec 5, 2020
@mhevery mhevery closed this in e1fe9ec Dec 5, 2020
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(common): use ngDevMode to tree-shake warnings (#39964)
f022efa 
This commit adds ngDevMode guard to show warnings only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake these warnings from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.

PR Close #39964
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(animations): use ngDevMode to tree-shake warning (#39964)
9ebe423 
This commit adds ngDevMode guard to show warning only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake this warning from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.

PR Close #39964
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(forms): use ngDevMode to tree-shake _ngModelWarning (#39964)
7954c8d 
This commit adds `ngDevMode` guard to call `_ngModelWarning` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this function from production builds
(since it will act as no-op, in dev mode everything will work as it works right now)
to decrease production bundle size.

PR Close #39964
@arturovt arturovt deleted the guard-check-no-changes-with-ng-dev branch December 5, 2020 00:08
@mhevery mhevery added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Dec 5, 2020
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(core): use ngDevMode to tree-shake checkNoChanges (#39964)
2fbb684 
This commit adds `ngDevMode` guard to run `checkNoChanges` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this code from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.

PR Close #39964
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(common): use ngDevMode to tree-shake warnings (#39964)
bf3de9b 
This commit adds ngDevMode guard to show warnings only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake these warnings from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.

PR Close #39964
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(animations): use ngDevMode to tree-shake warning (#39964)
72aad32 
This commit adds ngDevMode guard to show warning only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The ngDevMode flag helps to tree-shake this warning from production builds
(in dev mode everything will work as it works right now) to decrease production bundle size.

PR Close #39964
mhevery pushed a commit that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(forms): use ngDevMode to tree-shake _ngModelWarning (#39964)
735556d 
This commit adds `ngDevMode` guard to call `_ngModelWarning` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this function from production builds
(since it will act as no-op, in dev mode everything will work as it works right now)
to decrease production bundle size.

PR Close #39964
mhevery pushed a commit to mhevery/angular that referenced this pull request Dec 5, 2020
@arturovt @mhevery
perf(forms): use ngDevMode to tree-shake _ngModelWarning (angular…
5b47085 
…#39964)

This commit adds `ngDevMode` guard to call `_ngModelWarning` only
in dev mode (similar to how things work in other parts of Ivy runtime code).
The `ngDevMode` flag helps to tree-shake this function from production builds
(since it will act as no-op, in dev mode everything will work as it works right now)
to decrease production bundle size.

PR Close angular#39964
@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 Jan 5, 2021
@pullapprove pullapprove bot removed area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime area: forms area: performance labels Jan 5, 2021
@ngbot ngbot bot removed this from the Backlog milestone Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
No open projects
mhevery-review-queue
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arturovt @AndrewKushnir @mhevery