Skip to content

refactor(stepper): remove dependency on @angular/forms #15134

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
Merged
merged 1 commit into from
Mar 5, 2019
Merged

refactor(stepper): remove dependency on @angular/forms #15134

merged 1 commit into from
Mar 5, 2019

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Feb 9, 2019

While looking at #15085, I noticed that the only place in the CDK where we use @angular/forms is to type an optional input in the stepper. These changes replace the typing with a partial representation of the AbstractControl so that we can drop the dependency.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Feb 9, 2019
@crisbeto crisbeto requested a review from mmalerba as a code owner February 9, 2019 15:49
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 9, 2019
@mmalerba
Copy link
Contributor

mmalerba commented Feb 9, 2019

I think this is technically a breaking change. Anyone who is extending and trying to use other properties of AbstractControl in their derived class would need to add a line re-declaring the property.

I also just wonder if this is really worth it, I would expect the CDK to have other forms related stuff in the future (e.g. some common component for displaying error messages)

@crisbeto
Copy link
Member Author

crisbeto commented Feb 9, 2019

I can expand the typing to include the rest of the properties from AbstractControl or we can bump this to a major. As for other components potentially needing it in the future, IMO we should make the decision on whether to add it as a dependency when we get to it.

jelbourn
jelbourn approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker 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: merge The PR is ready for merge by the caretaker labels Feb 20, 2019
@jelbourn
Copy link
Member

jelbourn commented Feb 20, 2019

@crisbeto actually, can you update the BUILD.bazel file to remove the forms dependnecy?

Caretaker note: we need to remove this dependency in the Google build file as well

@crisbeto
refactor(stepper): remove dependency on @angular/forms
5189898 
While looking at angular#15085, I noticed that the only place in the CDK where we use `@angular/forms` is to type an optional input in the stepper. These changes replace the typing with a partial representation of the `AbstractControl` so that we can drop the dependency.
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Feb 20, 2019
@crisbeto crisbeto force-pushed the cdk-forms-dependency branch from 5014a46 to 5189898 Compare February 20, 2019 06:54
@josephperrott josephperrott merged commit f4a59f0 into angular:master Mar 5, 2019
josephperrott pushed a commit that referenced this pull request Mar 5, 2019
@crisbeto @josephperrott
refactor(stepper): remove dependency on @angular/forms (#15134)
173a682 
While looking at #15085, I noticed that the only place in the CDK where we use `@angular/forms` is to type an optional input in the stepper. These changes replace the typing with a partial representation of the `AbstractControl` so that we can drop the dependency.
@rcketscientist
Copy link

rcketscientist commented Mar 13, 2019

Unannounced breaking change. Thanks for that!

@rcketscientist rcketscientist mentioned this pull request Mar 13, 2019
Closed
@crisbeto
Copy link
Member Author

crisbeto commented Mar 13, 2019

@rcketscientist the assumption was that it shouldn't be a breaking change and none of the apps in Google that use Material hit the issue. What error did you get?

@rcketscientist
Copy link

rcketscientist commented Mar 13, 2019

error TS2339: Property 'value' does not exist on type '{ valid: boolean; invalid: boolean; pending: boolean; reset: () => void; }'.

As you mentioned in the PR it didn't map the full API. We use CDK extensively for many material-like widgets that have slightly different functionality/styling.

@crisbeto
Copy link
Member Author

crisbeto commented Mar 13, 2019

I see. I'll submit a hotfix for it. Sorry for the breaking change.

@rcketscientist
Copy link

rcketscientist commented Mar 13, 2019

Much appreciated :)

crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 13, 2019
@crisbeto
fix(stepper): avoid breaking change in stepControl type
ba77f73 
In angular#15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes angular#15462.
@crisbeto crisbeto mentioned this pull request Mar 13, 2019
Merged
crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 13, 2019
@crisbeto
fix(stepper): avoid breaking change in stepControl type
d16d8cf 
In angular#15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes angular#15462.
mmalerba pushed a commit that referenced this pull request Mar 13, 2019
@crisbeto @mmalerba
fix(stepper): avoid breaking change in stepControl type
a4f2900 
In #15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes #15462.
mmalerba pushed a commit that referenced this pull request Mar 13, 2019
@crisbeto @mmalerba
fix(stepper): avoid breaking change in stepControl type (#15464)
a146499 
In #15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes #15462.
mmalerba pushed a commit that referenced this pull request Mar 18, 2019
@crisbeto @mmalerba
fix(stepper): avoid breaking change in stepControl type (#15464)
f02ebe1 
In #15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes #15462.
@rcketscientist
Copy link

rcketscientist commented Mar 18, 2019

Thanks for the lightning fast patch guys!

Suresh918 pushed a commit to Suresh918/material2 that referenced this pull request Apr 15, 2019
@crisbeto
fix(stepper): avoid breaking change in stepControl type (angular#15464)
81a5ebf 
In angular#15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change.

Fixes angular#15462.
devversion added a commit to devversion/material2 that referenced this pull request Jun 24, 2019
@devversion
fix(stepper): unable to pass FormGroup as step control with strict ty…
0d5c036 
…pe checking

Before angular#15134 landed, the stepper accepted an`AbstractControl`
from `@angular/forms`. Unfortunately this PR accidentally changed
the type for the `stepControl` input from `AbstractControl` to `FormControl`.

This means that with strict template type checking, passing a `FormGroup`
(like we do in our dev-app various times) causes type check failures.

We need to adjust the `stepControl` type to accept an abstract control
which is the base of a `FormControl` and `FormmGroup`.
devversion added a commit to devversion/material2 that referenced this pull request Jun 24, 2019
@devversion
fix(stepper): unable to pass FormGroup as step control with strict ty…
eb9a47e 
…pe checking

Before angular#15134 landed, the stepper accepted an`AbstractControl`
from `@angular/forms`. Unfortunately this PR accidentally changed
the type for the `stepControl` input from `AbstractControl` to `FormControl`.

This means that with strict template type checking, passing a `FormGroup`
(like we do in our dev-app various times) causes type check failures.

We need to adjust the `stepControl` type to accept an abstract control
which is the base of a `FormControl` and `FormmGroup`.
devversion added a commit to devversion/material2 that referenced this pull request Jun 24, 2019
@devversion
fix(stepper): unable to pass FormGroup as step control with strict ty…
23d0109 
…pe checking

Before angular#15134 landed, the stepper accepted an`AbstractControl`
from `@angular/forms`. Unfortunately this PR accidentally changed
the type for the `stepControl` input from `AbstractControl` to `FormControl`.

This means that with strict template type checking, passing a `FormGroup`
(like we do in our dev-app various times) causes type check failures.

We need to adjust the `stepControl` type to accept an abstract control
which is the base of a `FormControl` and `FormmGroup`.
devversion added a commit to devversion/material2 that referenced this pull request Jun 25, 2019
@devversion
fix(stepper): unable to pass FormGroup as step control with strict ty…
39be887 
…pe checking

Before angular#15134 landed, the stepper accepted an`AbstractControl`
from `@angular/forms`. Unfortunately this PR accidentally changed
the type for the `stepControl` input from `AbstractControl` to `FormControl`.

This means that with strict template type checking, passing a `FormGroup`
(like we do in our dev-app various times) causes type check failures.

We need to adjust the `stepControl` type to accept an abstract control
which is the base of a `FormControl` and `FormmGroup`.
devversion added a commit to devversion/material2 that referenced this pull request Jun 26, 2019
@devversion
fix(stepper): unable to pass FormGroup as step control with strict ty…
e7ca296 
…pe checking

Before angular#15134 landed, the stepper accepted an`AbstractControl`
from `@angular/forms`. Unfortunately this PR accidentally changed
the type for the `stepControl` input from `AbstractControl` to `FormControl`.

This means that with strict template type checking, passing a `FormGroup`
(like we do in our dev-app various times) causes type check failures.

We need to adjust the `stepControl` type to accept an abstract control
which is the base of a `FormControl` and `FormmGroup`.
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 10, 2019

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

Reviewers

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@jelbourn jelbourn

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@crisbeto @mmalerba @jelbourn @rcketscientist @googlebot @josephperrott