adopted-ember-addons / ember-pikaday

A datepicker component for Ember CLI projects.
MIT License
159 stars 169 forks source link

Automatically updating values to the min/max date creates exceptions and potential infinite render in Ember Apps. #581

Closed Duder-onomy closed 5 months ago

Duder-onomy commented 5 months ago

This issue occurs on the head of master.

There are only a handful of things that Ember-pikday does which Pikaday does not do. Specifically, this code here In the modifier's modify hook: https://github.com/adopted-ember-addons/ember-pikaday/blob/c3a00669a99c2c3af1a66f815bee2f4bd5fcce4a/src/modifiers/pikaday.js#L60

if (minDate && value && value < minDate) {
  value = minDate;
  valueAltered = true;
}

As detailed in the readme here, this code is a departure from Pikday, and its intention is to update the value to the minDate if the value is earlier than the minDate.

^ Since this code runs on the first modify and potentially the first render. It can cause infinite rendering issues as it will update a value in the same 'computation' or 'runloop' as the value is read.

This creates bugs in Ember apps.

image

( Here ^ when using changesets )

I suggest we remove these lines ( and the corresponding lines for maxDate ) as:

I am happy to pull request this change if there is consensus.

FWIW: Ember 4.12 Ember-pikaday latest master

Duder-onomy commented 5 months ago

Doing some digging, If you pull down the master branch, a fresh pnpm install + pnpm test... the tests will fail with this same error:

image

So. it seems like this behavior is also breaking the master branch of Ember-Pikaday

MelSumner commented 5 months ago

@Duder-onomy if you want to create a PR, I'd be happy to review it.

Duder-onomy commented 5 months ago

@MelSumner I was able to open what I think is the extent of the changes we need: https://github.com/adopted-ember-addons/ember-pikaday/pull/593 Running that code in my application solves the exceptions I was seeing.
Happy to update the PR with anything else you think we might need.
Tks for all your effort in the Ember world BTW!! Really do appreciate it.

MelSumner commented 5 months ago

Okay, release v.5.0.0, see if that fixes it for ya! TY for doing the work! https://github.com/adopted-ember-addons/ember-pikaday/releases/tag/v5.0.0-ember-pikaday

Duder-onomy commented 5 months ago

v5.0.0 works great. Thank you!