getMomentDate can return null, at which point this would throw an error.

It would be good to document any function we introduce, as it may make this sort of thing easier to catch, when recognizing that the return value of the function is nullable.

Unit tests, as well, would be good to capture this sort of issue and the expected behavior.

Eesh, good call. I've gone back to the original implementation here and included a comment about to explain why moment() is used if currentDate is undefined or null.

Unit tests added. Thanks

I'm glad to see tests, but for what it's worth these tests as written would not have caught the error described in my previous review, since there is zero coverage for onChangeMoment.

This worries me a bit; I feel like it will be prone to test fragility, where the time required to execute the test is such that moment() called here will produce a different time than moment() called within the implementation of onChangeMoment. Usually this should take a mere fraction of a second, but I've witnessed in the past where it's enough time to tick into the next second and cause an uenxpected test failure.

I think our options would be:

• Find a way to provide or otherwise control the current time as referenced within onChangeMoment
• Mock moment to have full control over what it returns†
• Provide some flexibility in the assertion (i.e. within a +/- allowable timeframe)
• Don't test it

Lets go with Provide some flexibility in the assertion (i.e. within a +/- allowable timeframe) for $1000 please, Alex.

I figured comparison on basis of seconds was enough to overcome differences in execution time, but in an effort to prevent future headaches, a change is a good idea.

I changed the tests to save the argument passed to onChange for later comparison using moment's isSame function. Then I compare with a granularity of minute, which would preclude all but the rarest of circumstances where a pause in execution time ticks into the next second AND minute.