fix(@clayui/date-picker): remove moment.js

[bryceosterhaus]

I went with a POC in removing moment by replacing it with luxon date-fns. We still have a dependency, but its about 50kb smaller than when we used moment.

One caveat is that the "format" strings are different, so that would be a breaking change, although we could create some sort of map.

Additionally, I want to see if it'd be possible to just use native APIs, so I will continue on that and see what it requires.

Curious to hear any thoughts on this

1st commit is using luxon

2nd commit is using date-fns

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0252a2d:

Sandbox	Source
epic-zhukovsky-lt7jj	Configuration
epic-kirch-rl33b	Configuration

[bryceosterhaus]

Only issue with date-fns is the lack of localization support, but that only really effects our helper function getLocaleProps which I don't think is actually used anywhere...

[bryceosterhaus]

Going off of size-limit builds, with all dependencies, minified and gzipped

Clay using...	Total SIze
moment.js	120.51 KB
luxon	73.69 KB
date-fns	71.18 KB

[wincent]

Additionally, I want to see if it'd be possible to just use native APIs instead of luxon, so I will continue on that and see what it requires.

Glad you mention this as a possibility. If it could be made to work, that would clearly be a win in terms of size, isolation from the vagaries of third party stuff, and extracting maximum value from the platform.

[jbalsas]

Additionally, I want to see if it'd be possible to just use native APIs instead of luxon, so I will continue on that and see what it requires.

I would much rather prefer this.

If not possible, I would also prefer to see date-fns in here since it's the library we've been heavily using in DXP.

If, after all, we think luxon is the right choice, we should at least compare one with the other, pick based on merits and properly document our recommendation and support.

[bryceosterhaus]

Alright so I don't think we can get away with strictly native APIs since we allow for the custom date formatting... However, I was able to use just the format and parse functions from date-fns instead of the whole package.

Total size with date-fns: 62.67 KB (9.2KB from date-fns)

Knowing that we only need parse and format, I stumbled across dayjs, which is smaller again and has the same format patterns as moment. So in the second commit, I tried that out and it seemed to work great.

Total size with dayjs: 55.94 KB 🔥🔥 (2.6KB from dayjs)

Since we really don't rely on heavy date manipulation, I think going with dayjs would be appropriate for our use case. The only reason I see it appropriate to use date-fns instead is because its already in dxp.

Both libraries score well on ClearlyDefined(licensing) and both seem to be highly supported and popular in the community.

Overall, this would take clays total size from 120KB to 56KB. 😱

[jbalsas]

The only reason I see it appropriate to use date-fns instead is because its already in dxp.

I would then stick with date-fns for now. This is already a huge improvement and also reduces the changes we need to apply across the board... once settled we can go for dayjs or native once that's possible...

[bryceosterhaus]

Removed dayjs and going with date-fns.

Any final issues before merging other than docs?

[jbalsas]

I haven't checked code-wise, but I agree on principle! ;)

[wincent]

If you write it like this, it becomes obvious that these are just re-exports and not used in this file:

export {default as formatDate} from 'date-fns/format';
export {default as parseDate} from 'date-fns/parse';

[wincent]

I don't think we should always export functions in the same way. You have both:

export function a() {
  // ...
};

and:

export const b = () => {
  // ...
};

(FWIW, I prefer the former because of hosting.)

[wincent]

Edit (sigh): of course I meant "I do think"...

[wincent]

I created https://github.com/liferay/eslint-config-liferay/issues/184 to track enforcing this, with the recommendation that we go with export function — but feel free to comment there if you want to advocate the opposite (either way, we should enforce it).

[wincent]

So moment is still in here; is it coming in transitively? What does yarn why moment say?

[bryceosterhaus]

It comes from gatsby.

   - "_project_#clayui.com#gatsby" depends on it
   - Hoisted from "_project_#clayui.com#gatsby#moment"
   - Hoisted from "_project_#@clayui#css#metalsmith-permalinks#moment"

[bryceosterhaus]

@wincent for some reason when I update the syntax to be export {default as formatDate} from 'date-fns';, jest fails. I tried to update both jest and ts but no luck. Have you seen this before?

[wincent]

Have you seen this before?

Might be a Babel thing? If you look at the distributed source of data-fns/format/index.js, it looks like it is supposed to work:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = format;
// ... etc

Maybe something that will magically start working in the future at some point when some dep gets updated.

[bryceosterhaus]

@wincent I was hoping it wouldve resolved after the babel dependency updates but seems to still break. I'm just gonna import and then export separately for now and then look more into how our dependencies might be causing this.