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(kit): ComboBox throws ExpressionChangedAfterItHasBeenCheckedError #1772

Merged
merged 1 commit into from
May 17, 2022

Conversation

nsbarsukov
Copy link
Member

@nsbarsukov nsbarsukov commented May 17, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactoring
  • Code style update
  • Build or CI related changes
  • Documentation content changes

What is the current behavior?

Simplified version of the bug’s reproduction:

HTML
<tui-root>
  <tui-combo-box
    [ngModel]="selectedProcedure"
    [stringify]="stringify"
    (searchChange)="onSearchProcedureChange()"
  >
    Type "1"

    <tui-data-list-wrapper
      *tuiDataList
      [items]="procedures$ | async"
    ></tui-data-list-wrapper>
  </tui-combo-box>
</tui-root>
Typescript
import { Component } from '@angular/core';
import { BehaviorSubject } from 'rxjs';


@Component({
  selector: 'my-app',
  templateUrl: './app.component.html',
})
export class ExampleComponent {
  procedures$ = new BehaviorSubject<any[]>([]);
  selectedProcedure: any = null;

 onSearchProcedureChange(): void {
    this.procedures$.next([
      { id: '1', name: 'one' },
      { id: '2', name: 'two' },
    ]);
  }

  stringify(procedure: any): string {
    return procedure?.id;
  }
}

What's happening

  1. When we type "1", this function is triggered:
    https://github.com/Tinkoff/taiga-ui/blob/d82ab19a9598ec47469b0d218b514c4ddd78263e/projects/kit/components/combo-box/combo-box.component.ts#L181-L198

Controller is not changed (it was null and after this function it is still null). It is still pristine.

No item is selected because procedures$ (inside our ExampleComponent) is empty. After that function onSearchProcedureChange (inside our ExampleComponent) is triggered and fills procedures$ with two new items (prop [items] inside <tui-data-list-wrapper /> changes).

  1. Tick happens! (all next steps happens in the same tick).

  2. Angular evaluates pristine-status as true inside this built-in form-controller directive.

  3. Angular renders new SelectOption-s and their ngOnInit were triggered:
    https://github.com/Tinkoff/taiga-ui/blob/68ab06940351262f9468165d5c4a3aa1046d5696/projects/kit/components/select-option/select-option.component.ts#L52-L56

They invoke checkOption-functions of the parent component.
https://github.com/Tinkoff/taiga-ui/blob/d82ab19a9598ec47469b0d218b514c4ddd78263e/projects/kit/components/combo-box/combo-box.component.ts#L148-L155

And this.updateValue(option) makes control dirty.

  1. Angular starts its next digest cycle with checks (dev-mode only). At the start of the change-detection cycle pristine was true but at the end it is false. Angular throws error.

Closes #1643

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

@lumberjack-bot
Copy link

lumberjack-bot bot commented May 17, 2022

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

@github-actions
Copy link
Contributor

Visit the preview URL for this PR (updated for commit 8f66673):

https://taiga-ui--pr1772-combobox-expression-1w7vvjhy.web.app

(expires Wed, 18 May 2022 09:55:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1772 (8f66673) into main (dbf83f7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1772   +/-   ##
=======================================
  Coverage   63.26%   63.26%           
=======================================
  Files         922      922           
  Lines       10014    10015    +1     
  Branches     1935     1935           
=======================================
+ Hits         6335     6336    +1     
  Misses       3229     3229           
  Partials      450      450           
Flag Coverage Δ
addon-charts 74.42% <ø> (ø)
addon-doc 25.82% <ø> (ø)
addon-editor 45.87% <ø> (ø)
addon-mobile ∅ <ø> (∅)
addon-table 81.39% <ø> (ø)
addon-tablebars ∅ <ø> (∅)
cdk 69.63% <ø> (ø)
core 67.78% <ø> (ø)
kit 63.08% <100.00%> (+<0.01%) ⬆️
summary 63.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...omponents/select-option/select-option.component.ts 87.50% <100.00%> (+0.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbf83f7...8f66673. Read the comment docs.

@waterplea waterplea merged commit 2e22f4f into main May 17, 2022
@waterplea waterplea deleted the combobox-expression-changed-error branch May 17, 2022 14:41
@well-done-bot
Copy link

well-done-bot bot commented May 17, 2022

'Well done'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

🐞 - ComboBox throws ExpressionChangedAfterItHasBeenCheckedError
3 participants