chore(input): switch to OnPush change detection #5692
Conversation
googlebot
added
the
cla: yes
src/lib/input/input-container.ts
Outdated
|
|
||
| /** Whether the element is focused or not. */ | ||
| focused = false; | ||
|
|
||
| /** Sets the aria-describedby attribute on the input for improved a11y. */ | ||
| ariaDescribedby: string; | ||
|
|
||
| /** Stream that emits whenever one of the input states changes. */ |
There was a problem hiding this comment.
Expand this comment to include why this is necessary at all?
src/lib/input/input-container.ts
Outdated
|
|
||
| _onBlur() { this.focused = false; } | ||
| ngDoCheck() { | ||
| if (this._ngControl) { |
There was a problem hiding this comment.
Ah very cool, this is what I was talking about when I said forms doesn't always give us a way to know when we need to update, but this is a cool workaround :)
There was a problem hiding this comment.
Happy to close #5660 since it looks like this takes care of that 👍
| if (this._ngControl) { | ||
| // We need to re-evaluate this on every change detection cycle, because there are some | ||
| // error triggers that we can't subscribe to (e.g. parent form submissions). This means | ||
| // that whatever logic is in here has to be super lean or we risk destroying the performance. |
There was a problem hiding this comment.
Would it make sense to also add a note to the Custom Error Matcher section of input.md that mentions this?
Note: An input's error state must be calculated every change detection cycle, so it is important to keep the logic in your error state matcher very lean or you risk destroying performance.
There was a problem hiding this comment.
I'm not sure there is. FWIW, the code currently in master will be called even more times per change detection cycle since we have the _isErrorState in multiple places.
|
Addressed the feedback @jelbourn. @willshowell if nothing else, #5660 still provides some value by adding the extra unit test. |
jelbourn
added
pr: lgtm
action: merge
|
@crisbeto looks to be a few lint errors |
kara
added
pr: needs rebase
and removed
action: merge
|
@crisbeto Rebase? |
Switches the `MdInputContainer` to `OnPush` change detection and sorts out the various change detection-related issues. Relates to angular#5035.
crisbeto
added
action: merge
|
Rebased. |
|
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. |
Switches the
MdInputContainertoOnPushchange detection and sorts out the various change detection-related issues.Relates to #5035.