-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(common)!: migrate from moment
to moment-tiny
#1456
Merged
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
349cb26
feat(common): migrate back to moment repo to get ESM build
ghiscoding-SE 6504542
feat(common): migrate from `moment` to `moment-tiny`
ghiscoding 5712e58
chore: update pnpm lock file
ghiscoding e401eae
Merge branch 'master' into chore/moment-esm
ghiscoding 309ae54
Merge branch 'next' into chore/moment-esm
ghiscoding 56157e4
Merge branch 'next' into chore/moment-esm
ghiscoding 2ee634b
chore: fix build error after merging latest code
ghiscoding 0f1da01
chore: parse Moment dates with multiple formats
ghiscoding File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
packages/common/src/filter-conditions/__tests__/filterConditionProcesses.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a different behavior now with the date parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for unknown reason the newer version of moment doesn't accept zero padding anymore and in some ways it makes sense though, If we look at their Format docs, it does show only 1 digit when using
D
orM
. I could useDD
orMM
but I'm not sure if that would work and what I'm almost certain is that would probably fail in strict mode when zero padding is missing... I can give it another try tonight thoughI guess that what you're saying is that because of this, I might have to wait until next major and advertise the possible strictness in next major, is that it?
ok so I went through all their recent PRs and found this commit introduced in v2.30.0 so it seems that it doesn't accept zero padding in strict mode only. I think I use strict mode when sorting and filtering
Sorting
slickgrid-universal/packages/common/src/sortComparers/dateUtilities.ts
Lines 8 to 16 in 5ab671b
Filtering
slickgrid-universal/packages/common/src/filter-conditions/dateFilterCondition.ts
Lines 11 to 20 in 5ab671b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zewa666 I gave it a try and it is as I thought, when having a date like
01/01/92
andM/D/YY
will not pass in strict mode. I think I should keep using strict mode in my lib nonetheless... So it really looks like MomentJS introduced a breaking change without even knowing and for that reason, I guess I have no choice but to postpone it for the next major release in 2 months and mention this new strictness in the migrationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zewa666 alright, so what I found is that to go around the more strictness parsing problem with the new MomentJS version, what we can do is to provide multiple formats via an array. So what I've done is to add 2 formats for all the date formats that are considered as short. For example with
Field.dateUsShort
, I'm now passing['MM/DD/YY', 'M/D/YY']
so that the user can still pass dates with/without zero padding. However there are 3 things to noteMM/DD/YY
More info can be found in this Moment doc:
https://momentjscom.readthedocs.io/en/stable/moment/01-parsing/04-string-formats/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome feature. just need something like that for the paste handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow you're not sleeping, I thought you would reply in a day or two 😋
do you mean for another feature like the copy handler or did you mean something else? Should I go ahead with the merge then? what is your thought on the subject? I guess I'll be ready for a beta version by next week :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kids just woke me up ;)
yeah we've just experienced during the POC that handling date pastes might be tricky as people usually have excel files and stuff whatever they found right into the grid. So having the combo of a couple of supported formats + a locale is a great option.
I think this is definitely good for merge.
going to have another week to focus on the grid in 2w so looking forward to incorporate the new releases and fixes 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I wish that I could have switched to date-fns but it always seems that MomentJS is still the best and no other lib have all the bells and whistles of Moment.
So I might have a beta by next week but it's still earlier than Angular 18 (which is scheduled for around May 20), however I will still make it the new minimum version requirement. I'm quite happy with the switch to pure CSS SVG, the Dark Mode is looking so so much better now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont look at me, still using moment and numeral in most projects bc it just works and the size overhead is ignorable in enterprise internal only apps. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with all of that