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 and improve Duration#normalize #1494

Merged
merged 11 commits into from
Aug 26, 2023

Conversation

diesieben07
Copy link
Collaborator

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 icambron merged commit 1a03b22 into moment:master Aug 26, 2023
1 check passed
@icambron
Copy link
Member

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

@diesieben07
Copy link
Collaborator Author

diesieben07 commented Aug 26, 2023

@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
Copy link
Member

icambron commented Aug 26, 2023

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
Copy link

Looks like this PR may break builds that use webpack 4 similarly to this: alpinejs/alpine#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Duration.normalize() not working properly after 3.4.1
3 participants