Sitelet https://web.archive.org/web/20221117221857/https://github.com/angular/angular/pull/47576
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

fix(forms): always call setDisabledState, not just when the control is disabled #47576

Closed
wants to merge 1 commit into from

Conversation

dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Sep 29, 2022

When a CVA is added to a form, reactive forms should always call the setDisabledState method, not just when it's disabled.

This PR provides an option in the forms module config to opt-out, since it's a breaking change. We will likely use that option inside of g3 if too many tests are broken.

Fixes #35309.

@dylhunn dylhunn added type: bug/fix state: WIP area: forms target: major This PR is targeted for the next major release labels Sep 29, 2022
@dylhunn dylhunn added this to the v15 feature freeze blockers milestone Sep 29, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch from 3c57a3e to f54323c Compare Sep 30, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch from f54323c to bd754a3 Compare Sep 30, 2022
@dylhunn dylhunn added the action: global presubmit A TGP is required and / or is running label Sep 30, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch 4 times, most recently from 82f6605 to f6f5e07 Compare Sep 30, 2022
@dylhunn dylhunn marked this pull request as ready for review Oct 3, 2022
@dylhunn dylhunn requested a review from AndrewKushnir Oct 3, 2022
@pullapprove pullapprove bot requested review from alxhub and atscott Oct 3, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch from f6f5e07 to 85ead29 Compare Oct 3, 2022
goldens/public-api/forms/index.md Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
goldens/public-api/forms/index.md Outdated Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/src/directives/shared.ts Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from AndrewKushnir Oct 3, 2022
@dylhunn dylhunn force-pushed the call-setdisabledstate branch 2 times, most recently from 64f2e6a to 10cdf83 Compare Oct 3, 2022
@dylhunn dylhunn added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 3, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

LGTM, just some minor comments below. Thank you.

packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
packages/forms/test/value_accessor_integration_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/value_accessor_integration_spec.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from AndrewKushnir Oct 3, 2022
packages/forms/src/directives/shared.ts Show resolved Hide resolved
packages/forms/src/directives/shared.ts Show resolved Hide resolved
packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested review from atscott and removed request for atscott Oct 3, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 11, 2022 •

The components breakage has now been fixed in angular/components#25790

@dylhunn dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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 Oct 11, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 11, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 11, 2022 •

merge-assistance: wait for CI to finish, then submit this together with g3 local mod

…trol is enabled

Previously, `setDisabledState` was never called when attached if the control is enabled. This PR fixes the bug, and creates a configuration option to opt-out of the fix.

Fixes angular#35309.

BREAKING CHANGE: setDisabledState will always be called when a `ControlValueAccessor` is attached. You can opt-out with `FormsModule.withConfig` or `ReactiveFormsModule.withConfig`.
@dylhunn dylhunn force-pushed the call-setdisabledstate branch from bea364e to 6ab382e Compare Oct 11, 2022
@pullapprove pullapprove bot requested a review from devversion Oct 11, 2022
@dylhunn dylhunn closed this Oct 11, 2022
@dylhunn dylhunn deleted the call-setdisabledstate branch Oct 11, 2022
@angular angular deleted a comment from ngbot bot Oct 11, 2022
@angular angular deleted a comment from ngbot bot Oct 11, 2022
@dylhunn dylhunn restored the call-setdisabledstate branch Oct 11, 2022
@dylhunn dylhunn reopened this Oct 11, 2022
Copy link
Member

@devversion devversion left a comment

LGTM for dev-infra

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

reviewed-for: size-tracking

@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Oct 11, 2022

This PR was merged into the repository by commit 96b7fe9.

regulas1 pushed a commit to regulas1/angular that referenced this pull request Oct 16, 2022
…trol is enabled (angular#47576)

Previously, `setDisabledState` was never called when attached if the control is enabled. This PR fixes the bug, and creates a configuration option to opt-out of the fix.

Fixes angular#35309.

BREAKING CHANGE: setDisabledState will always be called when a `ControlValueAccessor` is attached. You can opt-out with `FormsModule.withConfig` or `ReactiveFormsModule.withConfig`.

PR Close angular#47576
@dylhunn dylhunn deleted the call-setdisabledstate branch Oct 17, 2022
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Nov 17, 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 Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: forms forms: ControlValueAccessor forms: directives merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
5 participants