search

moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.33k stars 730 forks source link

Fix and improve `Duration#normalize` #1494

Closed diesieben07 closed 1 year ago

diesieben07 commented 1 year ago

Fixes #1493

In the process of fixing this, I have updated the algorithm used in normalizeValues to make it more clear what it actually does and improved the documentation for Duration#normalize to clarify what "canonical representation" means.

Additionally, I have removed the broken implementation of convert that was introduced in #1467. After the refactoring of normalizeValues, Duration#shiftTo was the only user of convert, and unnecessarily so. shiftTo used convert to perform some of the same steps that normalize already does, which shiftTo calls afterwards. As such convert has been removed.

icambron commented 1 year ago

@diesieben07 i went ahead and merged this, but i'm a little concerned that there are no new tests

diesieben07 commented 1 year ago

@icambron I looked at the existing tests and did not find any cases that were not covered.

However I can take a look again and try to cover more cases if you think it is important.

icambron commented 1 year ago

Ah, I'd missed that we had a broken test. That's because my local env has a few failing tests because I use a newer version of node (causes the format strings to not match up), and I think the real test failure got lost in it. I should have noticed in the CI though. I updated the test to use a newer Node.

Thanks for the fix, release in 3.4.2

s-palmer commented 1 year ago

Looks like this PR may break builds that use webpack 4 similarly to this: https://github.com/alpinejs/alpine/pull/3679

Changing this line: let sum = vals.milliseconds ?? 0; to: let sum = vals.milliseconds !== null && vals.millisecondsl !== undefined ? vals.milliseconds : 0

resolves the build issues