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

feat(common)!: migrate from moment to moment-tiny #1456

Merged
merged 8 commits into from
Apr 28, 2024
Merged

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Apr 5, 2024

  • so I created yet another repo, it is very similar to moment-mini except that it includes both CJS and ESM builds (which actually both come from the Moment project itself), however moment-mini package only references the CJS build even they build both CJS & ESM nonetheless (more info can be found on the moment-tiny repo)

Note

The newest version of MomentJS introduced a breaking change via this PR even if they released it under patch, it really is a breaking change (more strict on 1-2 digits). I think what I could do to ease the pain would be to add a new strictDateParse (defaulting to true) so that user could avoid the breaking change. The reason not to use non-strict is mostly it's way too loose by default which is why I prefer strict by default but providing a date of 01/01/23 with D/M/YY should still be accepted and it was prior to that referenced commit. The strict mode is mostly used in the Date Sorting and Filtering

TODOs

  • for the breaking change, another possibility is to provide not just 1 format but an array of formats that Moment will loop through internally for example ['M/D/YY', 'MM/DD'YY'] (however note that this could potentially impact perf unless 1st format is the correct one)
  • similar to previous task, another way would be to add an option to disable strict mode (when filtering/sorting)

ghiscoding-SE and others added 2 commits April 3, 2024 16:54
- in theory the `module` defined in the `package.json` is now linked to the ESM build as per this recent Moment [commit](moment/moment@87994b7)
- so I created yet another repo, it is very similar to `moment-mini` except that it includes both CJS and ESM builds (which actually both come from the Moment project), however `moment-mini` only includes the CJS build.
Copy link

stackblitz bot commented Apr 5, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.7%. Comparing base (3c22b3e) to head (0f1da01).
Report is 1 commits behind head on next.

Files Patch % Lines
packages/common/src/editors/dateEditor.ts 83.4% 1 Missing ⚠️
packages/common/src/filters/dateFilter.ts 87.5% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            next   #1456     +/-   ##
=======================================
- Coverage   99.8%   99.7%   -0.0%     
=======================================
  Files        197     197             
  Lines      21653   21652      -1     
  Branches    7066    7069      +3     
=======================================
- Hits       21589   21586      -3     
- Misses        58      60      +2     
  Partials       6       6             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding ghiscoding marked this pull request as draft April 5, 2024 02:02
const output = executeFilterConditionTest(options, getFilterParsedDates(searchTerms, FieldType.dateEuroShort));
expect(output).toBe(true);
});

it('should return False when input value equals the search terms min inclusive value and operator is set to "RangeExclusive"', () => {
const searchTerms = ['01/12/93..31/12/93'];
const options = { dataKey: '', operator: 'RangeExclusive', cellValue: '01/12/93', fieldType: FieldType.dateEuroShort, searchTerms } as FilterConditionOption;
const searchTerms = ['1/12/93..31/12/93'];
Copy link
Contributor

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?

Copy link
Owner Author

@ghiscoding ghiscoding Apr 5, 2024

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 or M. I could use DD or MM 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 though

I 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?

image

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

export function compareDates(value1: any, value2: any, sortDirection: number, format: string | moment_.MomentBuiltinFormat, strict?: boolean) {
let diff = 0;
if (value1 === value2) {
diff = 0;
} else {
// use moment to validate the date
let date1: moment_.Moment | Date = moment(value1, format, strict);
let date2: moment_.Moment | Date = moment(value2, format, strict);

Filtering

export function executeDateFilterCondition(options: FilterConditionOption, parsedSearchDates: any[]): boolean {
const filterSearchType = options && (options.filterSearchType || options.fieldType) || FieldType.dateIso;
const FORMAT = mapMomentDateFormatWithFieldType(filterSearchType);
const [searchDate1, searchDate2] = parsedSearchDates;
// cell value in moment format
const dateCell = moment(options.cellValue, FORMAT, true);
// return when cell value is not a valid date
if ((!searchDate1 && !searchDate2) || !dateCell.isValid()) {

Copy link
Owner Author

@ghiscoding ghiscoding Apr 5, 2024

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 and M/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 migration

Copy link
Owner Author

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 note

  1. I can only use 1 output format and so I will always prefer the format with zero padding MM/DD/YY
  2. parsing with multiple formats is slower than passing a single format and the array order matters, so zero padding will be quicker
  3. I use strict mode when filtering & sorting, another idea would be to loosen the parsing and not use strict mode but non-strict mode is often too loose and I prefer strict mode

More info can be found in this Moment doc:
https://momentjscom.readthedocs.io/en/stable/moment/01-parsing/04-string-formats/

Copy link
Contributor

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

Copy link
Owner Author

@ghiscoding ghiscoding Apr 28, 2024

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 😋

awesome feature. just need something like that for the paste handling

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 :)

Copy link
Contributor

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 🎉

Copy link
Owner Author

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 :)

Copy link
Contributor

@zewa666 zewa666 Apr 28, 2024

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. 😅

Copy link
Owner Author

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

@zewa666
Copy link
Contributor

zewa666 commented Apr 5, 2024

hah thats a nice coincidence. we just installed moment yesterday for the date parsing of our dynamic column formatstrings and adding cjs to the new vite build felt so wrong 😅

I'm gonna give it a try as well

@ghiscoding
Copy link
Owner Author

ghiscoding commented Apr 5, 2024

hah thats a nice coincidence. we just installed moment yesterday for the date parsing of our dynamic column formatstrings and adding cjs to the new vite build felt so wrong 😅

I'm gonna give it a try as well

Yeah so as I mentioned in the other discussion, as per this Moment commit, they transpile to ESM but if you look at their package.json, it's not even linked anywhere (except jsnext:main which I have never seen before). I would guess that it's because they don't want to break their user base. So I simply cloned their repo, copied the ESM build (from min/moment.min.js location) and copied the root one which is CJS or UMD maybe and created a repo with that (I think moment-mini were doing the same anyway but just the CJS version which I'm not that interested except when Jest is running which is still CJS). I also provide a better exports in the package as well, so in theory it should work better with Vite (I haven't fully tested it though but I no longer see the CJS warning in Angular at least which was my goal)

@ghiscoding ghiscoding changed the title feat(common): migrate from moment to moment-tiny feat(common)!: migrate from moment to moment-tiny Apr 7, 2024
@ghiscoding ghiscoding changed the base branch from master to next April 26, 2024 18:34
- the latest version of MomentJS is not as loose as previous version (especially since we use strict mode when filtering & sorting), however what we can do is provide multiple formats to accept with/without zero padding so that both ("1/2/98" and "01/02/98" would work).
- Note however that as mentioned in MomentJS docs
> Note: Parsing multiple formats is considerably slower than parsing a single format. If you can avoid it, it is much faster to parse a single format.
- so we'll do multiple formats only for formats that are considered shorts
@ghiscoding ghiscoding marked this pull request as ready for review April 28, 2024 04:42
@ghiscoding ghiscoding merged commit 90690f4 into next Apr 28, 2024
6 checks passed
@ghiscoding ghiscoding deleted the chore/moment-esm branch April 28, 2024 05:36
@ghiscoding
Copy link
Owner Author

@zewa666 have you had a chance to try moment-tiny with Vite? It looks like the min JS that I thought was an ESM build is not after all and I'm still getting the CJS in Angular and need to add it to allowed CJS. So I'm not sure if it's worth the move in that case.

@zewa666
Copy link
Contributor

zewa666 commented May 2, 2024

just created a fresh new empty vite project, installed moment-tiny and it loaded it from the esm folder.

image

@ghiscoding
Copy link
Owner Author

ghiscoding commented May 2, 2024

@zewa666 yeah ok but that's probably just because I did add an exports entry for ESM, however what I'm saying is that the build (file) in itself doesn't seem to be ESM at all because Angular still complains that it's CJS and for Jest I still have to use this ESM mock below for my unit tests to work (notice the use of __esModule: true which is to simulate an ESM package in a non-ESM package).

But anyway, does this new package help at all with Vite? is it better than using Moment (or moment-mini)?

// Jest has a hard time with MomentJS because they export as default, to bypass this problem we can mock the require .default
jest.mock('moment-tiny', () => {
const actual = jest.requireActual('moment-tiny');
return { __esModule: true, ...actual, default: actual };
});

@zewa666
Copy link
Contributor

zewa666 commented May 2, 2024

oh ok I see. I'll give it a try with angular and see how it goes

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.

3 participants