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.

Sign up for GitHub

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

refactor(forms): Move FormControl to an overridden exported constructor. #44806

Closed
wants to merge 1 commit into from
Closed

refactor(forms): Move FormControl to an overridden exported constructor. #44806

wants to merge 1 commit into from

Conversation

dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jan 24, 2022 •
edited
Loading

This implementation change was originally proposed as part of Typed Forms, and will have major consequences for that project as described in the design doc. Submitting it separately will greatly simplify the risk of landing Typed Forms. This change should have no visible impact on normal users of FormControl.

See the Typed Forms design doc here: https://docs.google.com/document/d/1cWuBE-oo5WLtwkLFxbNTiaVQGNk8ipgbekZcKBeyxxo.

This is similar to the previous PR #44316, which was rolled back due to breakage in g3.

PR Close #44806

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:

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ngbot ngbot bot modified the milestone: Backlog Jan 24, 2022
@dylhunn dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews aio: preview labels Jan 24, 2022
@mary-poppins
Copy link

You can preview 2a9d1aa at https://pr44806-2a9d1aa.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 769eb1d at https://pr44806-769eb1d.ngbuilds.io/.

@dylhunn
Copy link
Contributor Author

dylhunn commented Jan 25, 2022

I ran a TGP on the train today, and it seems to be green except a few flakes: link

@dylhunn dylhunn force-pushed the fc-override branch 5 times, most recently from 44bde4c to 451a968 Compare January 25, 2022 02:31
@dylhunn dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 25, 2022
@mary-poppins
Copy link

You can preview dd48cec at https://pr44806-dd48cec.ngbuilds.io/.

@dylhunn dylhunn marked this pull request as ready for review January 25, 2022 02:31
@mary-poppins
Copy link

You can preview 451a968 at https://pr44806-451a968.ngbuilds.io/.

gkalpak
gkalpak requested changes Jan 25, 2022
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Overall lgtm. Just some minor comments/suggestions.

packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/test/form_control_spec.ts Show resolved Hide resolved
aio/tools/transforms/angular-api-package/processors/mergeOverriddenImplementation.js Outdated Show resolved Hide resolved
aio/tools/transforms/angular-api-package/processors/mergeOverriddenImplementation.js Outdated Show resolved Hide resolved
@dylhunn dylhunn force-pushed the fc-override branch 2 times, most recently from 259f793 to e14468a Compare January 25, 2022 17:40
@dylhunn dylhunn requested a review from gkalpak January 25, 2022 17:41
@dylhunn
Copy link
Contributor Author

dylhunn commented Jan 25, 2022

Thank you George, I have replied to all your comments, and committed the fixes for most of them!

@dylhunn dylhunn force-pushed the fc-override branch 2 times, most recently from 2f6e206 to 5028400 Compare January 27, 2022 03:24
@mary-poppins
Copy link

You can preview 5028400 at https://pr44806-5028400.ngbuilds.io/.

@dylhunn dylhunn force-pushed the fc-override branch 2 times, most recently from 871b32e to 27b7545 Compare January 27, 2022 20:01
@mary-poppins
Copy link

You can preview 27b7545 at https://pr44806-27b7545.ngbuilds.io/.

AndrewKushnir
AndrewKushnir reviewed Jan 28, 2022
View reviewed changes
goldens/public-api/forms/forms.md Outdated Show resolved Hide resolved
packages/forms/src/model.ts Show resolved Hide resolved
@mary-poppins
Copy link

You can preview 9e5fad1 at https://pr44806-9e5fad1.ngbuilds.io/.

AndrewKushnir
AndrewKushnir approved these changes Jan 31, 2022
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.

Looks great, thanks @dylhunn 👍

packages/forms/src/model.ts Show resolved Hide resolved
AndrewKushnir
AndrewKushnir reviewed Jan 31, 2022
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.

@dylhunn quick question: could you please also check what do we have in the d.ts file (related to the FormControl) in the forms package with this change?

@dylhunn
Copy link
Contributor Author

dylhunn commented Jan 31, 2022

@dylhunn quick question: could you please also check what do we have in the d.ts file (related to the FormControl) in the forms package with this change?

@AndrewKushnir Sure! The available symbols are the same as in the .md file in this PR. Here is the whole file.

@dylhunn
refactor(forms): Move FormControl to an overridden exported construct…
8de8db9 
…or. (angular#44316)

This implementation change was originally proposed as part of Typed Forms, and will have major consequences for that project as described in the design doc. Submitting it separately will greatly simplify the risk of landing Typed Forms. This change should have no visible impact on normal users of FormControl.

See the Typed Forms design doc here: https://docs.google.com/document/d/1cWuBE-oo5WLtwkLFxbNTiaVQGNk8ipgbekZcKBeyxxo.

PR Close angular#44316
AndrewKushnir
AndrewKushnir approved these changes Jan 31, 2022
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.

Reviewed-for: public-api

@mary-poppins
Copy link

You can preview 8de8db9 at https://pr44806-8de8db9.ngbuilds.io/.

atscott
atscott approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@atscott atscott 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: public-api

packages/forms/src/model.ts Show resolved Hide resolved
aio/tools/transforms/angular-api-package/processors/mergeOverriddenImplementation.js Show resolved Hide resolved
@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 31, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit f0cfa00.

@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 Mar 3, 2022
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this pull request Apr 8, 2022
@dylhunn @josmar-crwdstffng
refactor(forms): Move FormControl to an overridden exported construct…
377b152 
…or. (angular#44316) (angular#44806)

This implementation change was originally proposed as part of Typed Forms, and will have major consequences for that project as described in the design doc. Submitting it separately will greatly simplify the risk of landing Typed Forms. This change should have no visible impact on normal users of FormControl.

See the Typed Forms design doc here: https://docs.google.com/document/d/1cWuBE-oo5WLtwkLFxbNTiaVQGNk8ipgbekZcKBeyxxo.

PR Close angular#44316

PR Close angular#44806
@dylhunn dylhunn deleted the fc-override branch November 30, 2022 20:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

@gkalpak gkalpak gkalpak approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: forms cross-cutting: types target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
6 participants
@dylhunn @mary-poppins @jessicajaniuk @atscott @gkalpak @AndrewKushnir