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

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

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 Dec 3, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 3, 2020

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

@AndrewKushnir AndrewKushnir commented Dec 3, 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...

@arturovt
Copy link
Contributor Author

@arturovt 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 added 2 commits Dec 4, 2020
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.
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.
@pullapprove pullapprove bot requested a review from AndrewKushnir Dec 4, 2020
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 Dec 4, 2020
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.
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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 requested a review from mhevery 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
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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

@AndrewKushnir AndrewKushnir commented Dec 4, 2020

Presubmit.

@AndrewKushnir AndrewKushnir added action: presubmit A standard presubmit is running / required and removed action: review labels Dec 4, 2020
mhevery
mhevery approved these changes Dec 5, 2020
Copy link
Contributor

@mhevery mhevery left a comment

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 A standard presubmit is running / required label Dec 5, 2020
@mhevery mhevery moved this from Waiting for author to Done in mhevery-review-queue Dec 5, 2020
@mhevery mhevery removed request for IgorMinar and alxhub 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 issue Dec 5, 2020
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 issue Dec 5, 2020
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 issue Dec 5, 2020
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 Dec 5, 2020
@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 issue Dec 5, 2020
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 issue Dec 5, 2020
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 issue Dec 5, 2020
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 issue Dec 5, 2020
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 issue Dec 5, 2020
…#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

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Jan 5, 2021

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
@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.
Labels
action: merge PR author is ready for this to merge cla: yes target: patch This PR is targeted for the next patch release
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants