X Tutup
The Wayback Machine - https://web.archive.org/web/20211022185114/https://github.com/angular/angular/pull/37821
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(forms): use ngDevMode to tree-shake error messages #37821

Closed
wants to merge 1 commit into from
Closed

perf(forms): use ngDevMode to tree-shake error messages #37821

wants to merge 1 commit into from

Conversation

@sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Jun 29, 2020 •
edited

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #37697

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@pullapprove pullapprove bot requested a review from AndrewKushnir Jun 29, 2020
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch from 7af7f75 to 94af248 Jun 29, 2020
@sonukapoor sonukapoor marked this pull request as draft Jun 29, 2020
@sonukapoor sonukapoor changed the title Guard form errors with ngdevmode fix(forms): use ngDevMode to tree-shake error messages Jun 29, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 30, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 30, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jun 30, 2020

FYI, I just did a quick test with a demo app and ReactiveForms module. The @angular/forms package got reduced from 41kb to 34kb (which is around 20% decrease) 🎉

We should consider creating a test app with all Forms features (Reactive, Template-driven, validators, etc) in the integration folder and have a size check setup there as well, so that we can monitor the effect of the changes performed in different PRs in the @angular/forms package size.

AndrewKushnir
AndrewKushnir reviewed Jun 30, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Thanks for these changes @sonukapoor, as I mentioned in my previous comment in the thread, it helps save around 7kb (or ~20%).

I've left a few comments, mainly around additional refactoring that can be made to avoid having noop functions (when the whole body of the function is guarded with ngDevMode check).

Another thing that we should consider is to make sure this works correctly with View Engine (since ngDevMode was mostly used in Ivy runtime code).

Thank you.

packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/radio_control_value_accessor.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/reactive_directives/form_control_directive.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_model.ts Show resolved Hide resolved
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch from 2e0cd30 to cb7cbdf Jul 16, 2020
AndrewKushnir
AndrewKushnir reviewed Jul 16, 2020
View changes
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch from d83aee6 to 2108d7a Jul 23, 2020
@AndrewKushnir AndrewKushnir removed this from the needsTriage milestone Jul 24, 2020
@AndrewKushnir AndrewKushnir added this to the v11-candidates milestone Jul 24, 2020
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch 2 times, most recently from ded1da7 to c6ec06b Jul 24, 2020
@sonukapoor sonukapoor marked this pull request as ready for review Jul 24, 2020
@sonukapoor sonukapoor requested a review from AndrewKushnir Jul 24, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 24, 2020 •
edited

Hi @sonukapoor, thanks for performing further updates! That's really great to see the dev-mode-only symbols go from the golden file 👍 I just left a quick comment on unimplemented function and also started tests in Google's codebase (+ global test run). Will keep you updated. Thank you.

@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch from c6ec06b to 778b820 Jul 25, 2020
@sonukapoor sonukapoor requested a review from clydin Jul 25, 2020
jelbourn
jelbourn approved these changes Aug 4, 2020
View changes
Copy link
Member

@jelbourn jelbourn left a comment

LGTM

Reviewed-for: public-api

clydin
clydin approved these changes Aug 5, 2020
View changes
@sonukapoor sonukapoor requested a review from AndrewKushnir Aug 5, 2020
@clydin
Copy link
Member

@clydin clydin commented Aug 5, 2020

By using the example forms code found here in combination with a newly generated CLI application, a savings of ~7kb was observed for both View Engine and Ivy renderers.

One area that might be beneficial to investigate is to potentially change the View Engine startup process to guarantee that ngDevMode is defined. That should allow the removal of the undefined checks throughout this PR.

@AndrewKushnir AndrewKushnir removed this from the v11-candidates milestone Aug 12, 2020
@ngbot ngbot bot added this to the needsTriage milestone Aug 12, 2020
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch 2 times, most recently from 6d85f17 to 61e2a22 Aug 13, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Aug 13, 2020 •
edited

Presubmit #2 + Global Presubmit #2.

@sonukapoor
perf(forms): use internal `ngDevMode` flag to tree-shake error messag…
662af03 
…es in prod builds

This commit adds a guard before throwing any forms errors. This will tree-shake
error messages which cannot be minified. It should also help to reduce the
bundle size of the `forms` package in production by ~20%.

Closes angular#37697
@sonukapoor sonukapoor changed the title fix(forms): use ngDevMode to tree-shake error messages perf(forms): use ngDevMode to tree-shake error messages Aug 13, 2020
@sonukapoor sonukapoor force-pushed the guard-form-errors-with-ngdevmode branch from 61e2a22 to 662af03 Aug 13, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Aug 14, 2020

FYI, tests in Google's codebase (global presubmit) went well.

Also adding a link to similar changes in Components repo for visibility: angular/components#20146.

@mhevery mhevery closed this in 201a546 Aug 24, 2020
@sonukapoor sonukapoor deleted the guard-form-errors-with-ngdevmode branch Aug 24, 2020
subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this issue Aug 29, 2020
@sonukapoor @subratpalhar92
perf(forms): use internal ngDevMode flag to tree-shake error messag…
e8c96da 
…es in prod builds (angular#37821)

This commit adds a guard before throwing any forms errors. This will tree-shake
error messages which cannot be minified. It should also help to reduce the
bundle size of the `forms` package in production by ~20%.

Closes angular#37697

PR Close angular#37821
subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this issue Aug 31, 2020
@subratpalhar92
Revert "perf(forms): use internal ngDevMode flag to tree-shake erro…
b5169c0 
…r messages in prod builds (angular#37821)"

This reverts commit e8c96da.
subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this issue Aug 31, 2020
@sonukapoor @subratpalhar92
perf(forms): use internal ngDevMode flag to tree-shake error messag…
15b6c38 
…es in prod builds (angular#37821)

This commit adds a guard before throwing any forms errors. This will tree-shake
error messages which cannot be minified. It should also help to reduce the
bundle size of the `forms` package in production by ~20%.

Closes angular#37697

PR Close angular#37821
profanis added a commit to profanis/angular that referenced this issue Sep 5, 2020
@sonukapoor @profanis
perf(forms): use internal ngDevMode flag to tree-shake error messag…
46bfefe 
…es in prod builds (angular#37821)

This commit adds a guard before throwing any forms errors. This will tree-shake
error messages which cannot be minified. It should also help to reduce the
bundle size of the `forms` package in production by ~20%.

Closes angular#37697

PR Close angular#37821
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 24, 2020

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

@alfaproject alfaproject

@mhevery mhevery

@jelbourn jelbourn

@clydin clydin

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: forms refactoring target: major
Projects
None yet
Milestone
needsTriage
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@sonukapoor @AndrewKushnir @muhammedMoussa @mhevery @clydin @alfaproject @jelbourn @googlebot
X Tutup