The Wayback Machine - https://web.archive.org/web/20220610044716/https://github.com/angular/angular/pull/45271
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

fix(forms): Property renaming safe code #45271

Closed
wants to merge 1 commit into from
Closed

fix(forms): Property renaming safe code #45271

wants to merge 1 commit into from

Conversation

ananyahs96
Copy link
Contributor

@ananyahs96 ananyahs96 commented Mar 4, 2022

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?

It was causing ng-touched not to be applied in optimized mode/prod where the compiler runs the full set of optimizations and compiler checks. This behavior is likely the case with all other ng control statuses.

Issue Number: N/A

What is the new behavior?

ng-touched is now applied successfully in all javascript compilation modes.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla
Copy link

@google-cla google-cla bot commented Mar 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@pullapprove pullapprove bot requested a review from dylhunn Mar 4, 2022
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Mar 4, 2022 •
edited

Is there a corresponding issue for this PR? This is pretty surprising behavior if true -- would love to check out a repro.

EDIT: context is in b/222494799. This was escalated internally.

There are some test failures -- I'm working with Ananya on chat to resolve them.

@dylhunn dylhunn added comp: forms target: minor type: bug/fix labels Mar 4, 2022
@ngbot ngbot bot added this to the Backlog milestone Mar 4, 2022
@ngbot ngbot bot added this to the Backlog milestone Mar 4, 2022
@ngbot ngbot bot added this to the Backlog milestone Mar 4, 2022
dylhunn
dylhunn reviewed Mar 4, 2022
View changes
Copy link
Contributor

@dylhunn dylhunn left a comment •
edited

Thank you for your PR. I've left a few comments. You'll also want to rerun the formatter -- check the lint CI result for instructions.

packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Show resolved Hide resolved
packages/forms/src/directives/ng_control_status.ts Outdated Show resolved Hide resolved
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Mar 6, 2022 •
edited

Looks great! Just a few last things before this can be merged:

  • One more comment from Andrew above
  • Size tracking: goldens/size-tracking/integration-payloads.json: forms -> main must be updated from 156654 to 157357. (Unfortunately, this change causes some increase in the final bundle size.)
  • Squash the three commits. I suggest running git rebase -i HEAD~3, and marking the two most recent commits as squash , and the original commit as pick. (There are several ways to do it though.)

@ananyahs96
fix(forms): Property renaming safe code
064b917 
This fixes property renaming related issues in advanced closure compiler
optimizations.
@dylhunn dylhunn self-requested a review Mar 7, 2022
dylhunn
dylhunn approved these changes Mar 7, 2022
View changes
Copy link
Contributor

@dylhunn dylhunn left a comment

reviewed-for: fw-forms, size-tracking

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Mar 7, 2022 •
edited

@ananyahs96 We're waiting on one more approval for size-tracking (it takes two). It looks like you're ready for merge as well, let me know if that's not the case. g3 presubmit is running.

@dylhunn dylhunn added action: presubmit action: review labels Mar 7, 2022
@dylhunn dylhunn requested review from AndrewKushnir and removed request for jelbourn Mar 7, 2022
@ananyahs96
Copy link
Contributor Author

@ananyahs96 ananyahs96 commented Mar 7, 2022

@ananyahs96 We're waiting on one more approval for size-tracking (it takes two). It looks like you're ready for merge as well, let me know if that's not the case. g3 presubmit is running.

I am ready to merge. Thanks Dylan!

atscott
atscott approved these changes Mar 7, 2022
View changes
Copy link
Contributor

@atscott atscott left a comment

reviewed-for: size-tracking

@dylhunn dylhunn added action: merge and removed action: review action: presubmit labels Mar 7, 2022
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 7, 2022

This PR was merged into the repository by commit aa7b857.

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Apr 7, 2022

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 Apr 7, 2022
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
@ananyahs96 @josmar-crwdstffng
fix(forms): Property renaming safe code (angular#45271)
3d4adb3 
This fixes property renaming related issues in advanced closure compiler
optimizations.

PR Close angular#45271
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir

@atscott atscott

@dylhunn dylhunn

Assignees
No one assigned
Labels
action: merge comp: forms target: minor type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ananyahs96 @dylhunn @AndrewKushnir @atscott