feat(forms): Give form statuses a more specific type #42952
Closed
+38
−24
Conversation
dylhunn
force-pushed the
dylhunn:narrow-status
branch
2 times, most recently
from
86efb37
to
6d2a8e4
|
@AndrewKushnir, can you do an initial review before I send this to a wider audience? |
dylhunn
force-pushed the
dylhunn:narrow-status
branch
3 times, most recently
from
a91e5dc
to
6552be8
|
LGTM |
dylhunn
force-pushed the
dylhunn:narrow-status
branch
6 times, most recently
from
5bf1af8
to
31e2632
|
LGTM for api change, let's please make sure that this change is properly documented in the change log and release notes. And let's keep an eye on the community feedback just in case the change causes unexpected pain. Thank you! Reviewed-for: public-api |
|
LGTM reviewed-for: public-api |
|
Reviewed-for: public-api |
|
This is blocked until the final google3 fix is approved: cl/387384219 |
|
nice one |
Specifically: narrow the type used for form statuses from string to a union of possible statuses. Change the API methods from any to use the new type. This is a breaking change. However, as discussed in the PR, breakage seems minimal, and google3 has been prepped to land this. Background: we uncovered these any typings in the course of design work for typed forms. They could be fixed in a non-breaking manner by piggybacking them on top of the new typed forms generics, but it would be much cleaner to fix them separately if possible. BREAKING CHANGE: A new type called `FormControlStatus` has been introduced, which is a union of all possible status strings for form controls. `AbstractControl.status` has been narrowed from `string` to `FormControlStatus`, and `statusChanges` has been narrowed from `Observable<any>` to `Observable<FormControlStatus>`. Most applications should consume the new types seamlessly. Any breakage caused by this change is likely due to one of the following two problems: (1) the app is comparing `AbstractControl.status` against a string which is not a valid status; or, (2) the app is using `statusChanges` events as if they were something other than strings.
|
Final TAP train run is here: https://test.corp.google.com/ui#id=OCL:388451111:BASE:388689750:1628116632110:adba20fd Everything looking good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Make Form Statuses use stricter types.
Specifically: narrow the type used for form statuses from string to a union of possible statuses. Change the API methods from
anyto use the new type.This is a breaking change. However, as discussed below, breakage seems minimal, and google3 has been prepped to land this.
Background: we uncovered these
anytypings in the course of design work for typed forms. They could be fixed in a non-breaking manner by piggybacking them on top of the new typed forms generics, but it would be much cleaner to fix them separately if possible.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, form status is described with just a plain string, and the return type of
statusChangesisObservable<any>.Issue Number: N/A
What is the new behavior?
Form status uses a more specific type, which can only be a valid status string (specifically, the union of all possible statuses). Accordingly,
statusChangesand friends are strongly typed.Does this PR introduce a breaking change?
Yes, this is a breaking change: some consumers will expect the status to be an arbitrary string, and after this change, it must be a valid status string. I ran a TGP in google3, and there were 20k broken tests, but I was able to fix them all by addressing only seven root causes by hand. Most root causes were type confusion regarding the
anytypes we currently use instatusChanges(Googlers, see b/194913527). The very small number of root causes suggests external breakage will be minimal, and what breakage does occur is likely due to incorrect use ofstatusChanges. After discussion with Alex and Andrew K, we think that breakage should be minimal enough that no schematic is required.Here is the breaking change blurb from the commit message:
A new type called
FormControlStatushas been introduced, which is a union of all possible status strings for form controls.AbstractControl.statushas been narrowed fromstringtoFormControlStatus, andstatusChangeshas been narrowed fromObservable<any>toObservable<FormControlStatus>. Most applications should consume the new types seamlessly. Any breakage caused by this change is likely due to one of the following two problems: (1) the app is comparingAbstractControl.statusagainst a string which is not a valid status; or, (2) the app is usingstatusChangesevents as if they were something other than strings.Other information
TGP went green two days ago after making the fixes in google3.