didReceiveAttrs does not trigger onChange

[mlazze]

Fixes #115, #114.

Added a test too.

This implies that an external change to value shouldn't trigger onChange, which seems fair to me.

[RobbieTheWagner]

Thanks for the PR @staskij! Looking at the flatpickr docs, they seem to indicate that it won't trigger onChange unless you pass true in there. https://chmln.github.io/flatpickr/#api-setDate

If it doesn't work as advertised, we might want to have them fix it there.

My only concern with this is making sure all the locales and formats and everything work the same with this change. Does everything look the same generally or did you test that out?

[mlazze]

flatpickr#setDate uses

if (triggerChange !== false)
			triggerEvent("Change");

to check if onChange should be triggered, so Change triggers always but when false is passed.

I'm afraid there is a difference in formatting if not using altFormat:
https://ember-twiddle.com/a14d945276da2982172c12caf1cb5364/b0ccb57b7fa6572ec045a65ab933a68f67d0c6dd?openFiles=templates.application.hbs%2C
The version with false ignores dateFormat for some reason, but only when displaying, flatpickr config still uses the given dateFormat. (the awkward date formats used in the twiddle are only to ensure i'm setting something different from the default ones)

[RobbieTheWagner]

@staskij what if we switched didReceiveAttrs for didUpdateAttrs or maybe we shouldn't pass the value to the initial setup at all https://github.com/shipshapecode/ember-flatpickr/blob/master/addon/components/ember-flatpickr.js#L126. We could remove that and just allow the value to be set by didReceiveAttrs each time. We need to ensure formatting and everything works.

Perhaps we should think up all the possible cases of setting the value and ensure we have tests for each of them and they all behave as they should?

[mlazze]

I'm wrong about dateFormat being ignored in displaying. Using setDate() without false causes the "you modified twice" error before glimmer rerenders, so the date is not updated.

My understanding of flatpickr formats is that:

• altFormat is used (if apiInput == true) only for display purposes
• dateFormat is used to parse the date in various functions if the date is a string. If date is a Date, it ignores dateFormat. (so, flatpickr#setDate(new Date()) ignores dateFormat, while flatpickr#setDate('2010-11-12') tries to parse '2010-11-12' into a DateObject using dateFormat). It is also used as the second parameter of flatpickr events API, so that onChange receives 3 arguments, the first is an array of DateObjects, the second one is the latest date formatted according to dateFormat and the third is the flatpickr instance.

So, we should check that:

• we can change the value externally and the input value changes
• if we open the picker and we click on a day onChange gets called with the correct parameters
• onChange is called with dateStr in the correct format according to dateFormat

I'm not sure if we should check anything more about formats, this is mostly a wrapper around flatpickr, most of the data manipulation gets implemented by the user in onChange/onClose/other events.

[mlazze]

Sorry if I'm being overly verbose in this PR, but i'm trying to understand what we should do.

onChange receives the three arguments from the flatpickr onChange event API, so if we destructure the first argument in a onChange action (like in the last test i added) we have no problem, but if we use onChange=(action (mut value)), value then becomes the first argument of the onChange flatpickr event, so value becomes an array of DateObjects.
I don't think this should be the standard use-case: mut is expected to be used in an onChange, with the same argument as the value property. But now, value receives either a DateObject, a string formatted accordingly to the dateFormat property or an Array of DateObject/string, but onChange=(action (mut value) sets value to an Array of DateObjects. So, here are my ideas:

• document that value property accepts a string formatted with dateFormat, a DateObject or an Array of DateObject/string
• document that onChange=(action (mut value) sets value to an Array of DateObjects, so if a user wants a dateobject he has to use a custom action setting onChange=(action "something") and something action has to destructure the first argument/use the second one
• pass an additional first parameter to onChange which is the first/last selected date, so that onChange=(action (mut value)) sets value to a DateObject, while if we use onChange=(action "something"), something gets called with (lastDateObject: Date, selectedDates: Array, dateStr: string, instance: Flatpickr)

[RobbieTheWagner]

@staskij it should be expected to be an array at all times now, I believe. When flatpickr switched to supporting selecting multiple dates on one calendar, they defaulted to arrays everywhere. Definitely possible we have some tweaking to do on the ember-flatpickr side to support this. I agree that it should be consistent.

[GCheung55]

@staskij nice work and investigating!

Since onChange is passed an array of selectedDate(s) one way way I can think of that onChange=(action (mut value)) would still work is if the value had set function to handle the date objects in the array defined.

// Assume the value is a computed property with set:
value: Ember.computed({
    set(key, value) {
        const [date] = value;

        // convert that date object to a specific format.
        return moment(date).format('YYYY-MM-DD');
    }
});

[RobbieTheWagner]

@GCheung55 sounds logical to me, at least at first glance. Could you and @staskij work together on creating a solution that you both agree on and then I'll get it merged in? I won't have much time to look into this myself for a couple weeks, unfortunately, but if you both agree it's good to go, and formatting and everything still works, I'll happily merge 😃

[RobbieTheWagner]

@staskij @GCheung55 do you think you guys would have the time to collaborate on this and get this PR to a mutually accepted solution that handles all the things?

[mlazze]

Well, I believe this PR should handle the problem of not updating the value twice each time a new date gets selected, and I believe the tests I've added show that setting setDate(*, false) solves the problem and leaves value and onChange properties to the same behaviours they had before (the date formats/type weren't tested before, so they only depend on what flatpickr accepts as setDate and flatpickr events).

As per "what should we expect as input and what should we return as output", it should be discussed more, but without the setDate(*, false) this component cannot be used anywhere atm, as the basic use-case is broken. So I'm open to new discussions related to input and output formats in another pr/issue, but I'm thinking this pr should be small, fast and fix only the warning ember throws. I'm only adding to this pr a little bit of documentation on how to handle input and output on this very moment.

Let me know if solving this issue alone and discussing value/onChange formats in another pr is ok.

[RobbieTheWagner]

@staskij can you check what version of flatpickr you are on? This issue does not occur on the demo site. I'm inclined not to push a fix through that breaks other parts of the addon, like formatting etc.

[mlazze]

ember-flatpickr: 1.1.6 (twiddle: https://ember-twiddle.com/a14d945276da2982172c12caf1cb5364/ba84439ff5edbf4843c6f22ab95b5ba30b0f1236?openFiles=twiddle.json%2C)

The problem does not occur if in production mode, only in development mode:
emberjs/ember.js#13948

As a compromise, we currently only perform the detection in development mode and turned the deprecation message into a development-mode assertion (hard error). In production mode, the detection code is stripped and backtracking will not work.

EDIT: oh sorry, you meant flatpickr, yeah it's newer than 2.3.2, just can't find which version is twiddle pulling

[RobbieTheWagner]

@staskij look at http://shipshapecode.github.io/ember-flatpickr/

If you look in the console, notice nothing is printed from an onChange until you change the value by interacting with the flatpickr.

[RobbieTheWagner]

@staskij looks like the behavior did indeed change in flatpickr. https://github.com/chmln/flatpickr/blob/v2.3.2/src/flatpickr.js#L1259 Thanks to @GCheung55 for finding that!

So we may want to just discuss getting this fixed in flatpickr itself. You could lock your flatpickr to 2.3.2 for now, for a quick fix. I just don't have the time to do a lot of digging into this right now.

[RobbieTheWagner]

@staskij @GCheung55 seems like @chmln is going to revert the behavior flatpickr/flatpickr#666 (comment), thus potentially fixing this issue without need for this PR.

Might want to at least keep some of the tests introduced here though. Will follow up when the change has been made on the flatpickr side.

[RobbieTheWagner]

@staskij seems like the behavior has been reverted now. Can you please remove your changes here, but keep the relevant tests? Happy to merge the tests in to ensure this doesn't happen again 😄

[mlazze]

I rebased the pr, leaving only the 4 tests and the change in the README. This fails on travis as travis pulls flatpickr without the reverted behaviour, but as a new flatpickr version gets released they'll succeed.

Shouldn't we also upgrade package.json to use the to-be released flatpickr? Any version of flatpickr from 2.4 to 2.4.4 have the "setDate triggers onChange by default" behaviour, and so they cause the error.

[RobbieTheWagner]

@staskij yeah, we will update the version when they release it

[NullVoxPopuli]

did they do it? flatpickr/flatpickr@a033486

[RobbieTheWagner]

@NullVoxPopuli yeah, they changed it back, but have not released a new version yet

[RobbieTheWagner]

Updated flatpickr. Going to merge this, and adjust to add any tweaks necessary for the new flatpickr, then will get a new version of ember-flatpickr out ASAP.