refactor(material/core): no longer define stateChanges in mixinErrorState
#22875
Conversation
devversion
added
docs
google-cla
bot
added
the
cla: yes
| // The description is not specifically mentioning the error state, as classes using this | ||
| // mixin can/should emit an event in other cases too. | ||
| /** Emits whenever the component state changes. */ | ||
| readonly stateChanges = new Subject<void>(); |
There was a problem hiding this comment.
I've added the internal comment here because I was actually surprised about stateChanges being defined here. I've also wondered on whether it should be made docs-private or not. I'm unsure on whether we should move this out of the mixin (as a cleanup) and require the mixin base classes to define the stateChanges property, but it looks like this mixin is commonly used within form-fields.. I kept it public because there are other cases where we have stateChanges intentionally showing-up in the docs.
There was a problem hiding this comment.
Now that you mention it, it does sound weird that this is initialized in the mixin. Maybe it should be moved to all the places that apply the mixin instead? The interface should enforce that it's always defined.
There was a problem hiding this comment.
Yeah, I agree, I was surprised too and left this comment to start the conversation.
There was a problem hiding this comment.
@crisbeto I've went ahead and just made the changes as part of this PR.
| // The description is not specifically mentioning the error state, as classes using this | ||
| // mixin can/should emit an event in other cases too. | ||
| /** Emits whenever the component state changes. */ | ||
| readonly stateChanges = new Subject<void>(); |
There was a problem hiding this comment.
Now that you mention it, it does sound weird that this is initialized in the mixin. Maybe it should be moved to all the places that apply the mixin instead? The interface should enforce that it's always defined.
devversion
added
the
merge: preserve commits
devversion
force-pushed
the
docs/mixin-error-state
branch
from
260fbbe to
47baa6d
Compare
devversion
added
needs: discussion
|
I've extracted the first commit into a separate PR so that we can land the "docs fix" in a non-breaking way: #23010. This PR remains open for further discussion given it's technically a breaking change (although I doubt the risk is high here). |
devversion
force-pushed
the
docs/mixin-error-state
branch
from
47baa6d to
68cb7b5
Compare
devversion
changed the title
mixinErrorState
devversion
added
action: merge
devversion
force-pushed
the
docs/mixin-error-state
branch
2 times, most recently
from
f76ca5b to
79b3e09
Compare
devversion
force-pushed
the
docs/mixin-error-state
branch
from
79b3e09 to
8ebc46d
Compare
andrewseguin
removed
the
cla: yes
googlebot
added
the
cla: yes
devversion
force-pushed
the
docs/mixin-error-state
branch
from
8ebc46d to
1a25f4c
Compare
…orState` The error state mixin provided in `@angular/material/core` currently defines the `stateChanges` class member as part of the mixin. This is unexpected as consumers might deal with the `stateChanges` member differently. e.g. in some components the `stateChanges` field is intended to show up in the docs, or the JSDoc descripton varies. e.g. the observable could emit whenever form-field state changes, and it should be updated, or it emits always when something changes (e.g. even a component input which is not relevant for the form-field control). In general we want to avoid this member being defined in the mixin as the mixin is rather about the error state, and not defining a subject that can emit whenever "state" changes. BREAKING CHANGE: Previously the `mixinErrorState` mixin function defined a class member for `stateChanges`. This is no longer the case, and consumers need to provide the `stateChanges` class member themselves.
devversion
force-pushed
the
docs/mixin-error-state
branch
from
1a25f4c to
310fcb6
Compare
devversion
removed
needs: discussion
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The error state mixin provided in
@angular/material/corecurrentlydefines the
stateChangesclass member as part of the mixin. Thisis unexpected as consumers might deal with the
stateChangesmemberdifferently. e.g. in some components the
stateChangesfield isintended to show up in the docs, or the JSDoc descripton varies. e.g.
the observable could emit whenever form-field state changes, and it
should be updated, or it emits always when something changes (e.g.
even a component input which is not relevant for the form-field
control).
In general we want to avoid this member being defined in the mixin
as the mixin is rather about the error state, and not defining a
subject that can emit whenever "state" changes.