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(datepicker): ExpressionChangedAfterItHasBeenCheckedError #2462

Conversation

divdavem
Copy link
Member

Reuses data structures in the datepicker data model instead of building new objects each time the user navigates to a different month. As a consequence, DOM elements are no longer destroyed when changing month.

This fixes the following issue which occurred when BrowserAnimationsModule was used: when using the keyboard to navigate from one month to another, the focused element was destroyed during the change detection of the keyboard event, which synchronously triggered the focusout event in Chrome, leading to a change in the data model (and that change is still part of the data model only when BrowserAnimationsModule is used, because that module changes the behavior of Angular to not destroy views and their data model as quickly as when this module is not loaded), and this was causing the ExpressionChangedAfterItHasBeenCheckedError error.

Fixes #2408

@divdavem divdavem force-pushed the datePickerExpressionChangedAfterItHasBeenChecked branch from ab04a3d to d66c807 Compare June 18, 2018 13:34
@maxokorokov maxokorokov modified the milestones: 3.0.0, 2.1.2 Jun 18, 2018
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey,

It looks much better to me memory and dom-node-count wise!

A couple of comments:

  • when a test from datepicker-tools.spec fails it produces a 1000-line exception message, so it's hard to see which test failed exactly. Not sure what can be done with it though...
  • I have an alternative implementation of the buildMonths method (see comment in code)

WDYT?

@@ -87,37 +87,68 @@ export function buildMonths(
calendar: NgbCalendar, date: NgbDate, state: DatepickerViewModel, i18n: NgbDatepickerI18n,
force: boolean): MonthViewModel[] {
const {displayMonths, months} = state;
const newMonths = [];
const newFirstMonthStart = calendar.getNext(date, 'm', 0);
Copy link
Member

Choose a reason for hiding this comment

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

What about the following implementation?
Not sure it's more/less optimised, but it is shorter and puts less strain on my brain :)
(all your tests pass as before)

  // move old months to a temporary array
  const monthsToReuse = months.splice(0, months.length);

  // generate new first dates, nullify or reuse months
  const firstDates = Array.from({length: displayMonths}, (_, i) => {
    const firstDate = calendar.getNext(date, 'm', i);
    months[i] = null;

    if (!force) {
      const reusedIndex = monthsToReuse.findIndex(month => month.firstDate.equals(firstDate));
      // move reused month back to months
      if (reusedIndex !== -1) {
        months[i] = monthsToReuse.splice(reusedIndex, 1)[0];
      }
    }

    return firstDate;
  });

  // rebuild nullified months
  firstDates.forEach((firstDate, i) => {
    if (months[i] === null) {
      months[i] = buildMonth(calendar, firstDate, state, i18n, monthsToReuse.shift() || {} as MonthViewModel);
    }
  });

@divdavem
Copy link
Member Author

divdavem commented Jun 19, 2018

@maxokorokov Thank you for your review.
To solve the issue of the 1000-line exception message, maybe I can write a custom matcher instead of my expectSameMonthsDataStructure function that calls expect many times.
I agree your buildMonths function is more readable than mine (and probably a bit less efficient with the findIndex that is repeated for each month, but the impact is probably negligible, as the number of months should never be very high, probably less than 5 in most cases). I will replace mine by yours.

Reuses data structures in the datepicker data model instead of building
new objects each time the user navigates to a different month.
As a consequence, DOM elements are no longer destroyed when changing month.

This fixes the following issue which occurred when BrowserAnimationsModule was
used: when using the keyboard to navigate from one month to another, the
focused element was destroyed during the change detection of the keyboard
event, which synchronously triggered the focusout event in Chrome, leading
to a change in the data model (and that change is still part of the data
model only when BrowserAnimationsModule is used, because that module changes
the behavior of Angular to not destroy views and their data model as quickly
as when this module is not loaded), and this was causing the
ExpressionChangedAfterItHasBeenCheckedError error.

Closes ng-bootstrap#2408
@divdavem divdavem force-pushed the datePickerExpressionChangedAfterItHasBeenChecked branch from d66c807 to 9a93a98 Compare June 19, 2018 15:15
@divdavem
Copy link
Member Author

@maxokorokov I have updated the code to take your remarks into account. Here is the diff: divdavem@97124b8

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@divdavem
Copy link
Member Author

@maxokorokov Thank you!

@maxokorokov maxokorokov merged commit b47f981 into ng-bootstrap:master Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatePicker Error: ExpressionChangedAfterItHasBeenCheckedError
2 participants