Skip to content

/ popcode

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

Jump to bottom

Swap out moment for date-fns #1711

Open
wants to merge 1 commit into
base: master
from
Open

Swap out moment for date-fns #1711

wants to merge 1 commit into from

Conversation

@razzius
Copy link
Member

@razzius razzius commented Jul 12, 2019 •
edited

PROOF that the date functionality still works (time from now is displayed in projects list):

image

The other use case is formatting a date:

> new moment().format('MMM Do [at] h:mm A')
'Jul 11th at 7:43 PM'
> format(new Date(), 'MMM Do [at] h:mm A')
'Jul 11th at 7:43 PM'
@razzius
Copy link
Member Author

@razzius razzius commented Jul 12, 2019

The bundle sizes are actually a regression, probably because chrono-node (a natural language date parser) depends on moment.

Stats from env PROFILE_BUILD=true npm run preview
http://localhost:3000/profile/webpack-visualizer.html:

Before:
Stat 11.09 MB
Parsed 4.91 MB
Gzipped 1.3 MB

After:
Stat 11.49 MB
Parsed 5.09 MB
Gzipped 1.33 MB
@razzius
Copy link
Member Author

@razzius razzius commented Jul 12, 2019

The date parsing is occurring in AssignmentCreatorForm.jsx

I've never used the AssignmentCreator functionality. Just to make sure, can anybody comment on how it's part of the app, maybe with a helpful screenshot?
@razzius
Copy link
Member Author

@razzius razzius commented Jul 12, 2019

If we removed chrono-node, the bundle is as expected significantly smaller.

Stat 10.81 MB
Parsed 4.74 MB
Gzipped 1.25 MB
@razzius
Swap out moment for date-fns
Loading status checks…
24f9c3e
@razzius razzius force-pushed the razzius:razzi/date-fns branch from 384ca8c to 24f9c3e Jul 12, 2019
@outoftime
outoftime reviewed Jul 12, 2019
View changes
Copy link
Contributor

@outoftime outoftime left a comment

@razzius nice work/investigation! To answer your question, the assignment creator is an experimental feature and only appears when you load the page with the ?experimental query param in the URL (e.g. http://localhost:3000?experimental).

The assignment form component—and thus any of its dependencies—should be loaded asynchronously, though, so if the effect here is to move the bundle weight of moment out of the synchronous bundle, that is still an important improvement. It’s been a while since I have used the profiling tools, but IIRC one of them breaks things out by bundle and the other does not; my dev environment is not in a good state to explore myself at the moment. Can you confirm the above hypothesis?

There is an open issue on chrono-node that raises the problem with relying on moment, and it seems like the maintainer is open to switching to dayjs, which apparently has a moment-compatible API. Given that, maybe we should consider using dayjs in favor of date-fns? Also, it might be worth just contributing a PR to chrono-node to make the switch; if it’s API-compatible and the library has a solid test suite, I can’t imagine it would be a particularly onerous transition.

All that said, I am down to merge this as-is if we can confirm that moment is no longer contributing to the size of the critical bundle, and with the webpack config to exclude the moment locale files restored.
webpack.config.js
test: /\.js$/u,
include: matchModule('moment/locale'),
use: ['null-loader'],
},

This comment has been minimized.

Sign in to view
@outoftime

outoftime Jul 12, 2019
Contributor

I’m guessing this might be why the bundle size increased substantially? (Just adding date-fns should not move the needle much)
@outoftime outoftime added this to In progress in Performance Sep 3, 2019
@paulproteus
Copy link

@paulproteus paulproteus commented Aug 26, 2020

I see that chrono-node now depends on dayjs as its one dependency. https://github.com/wanasit/chrono/blob/master/package.json#L49 Perhaps someone else coming by this PR will find an interest in checking the size impact of upgrading chrono as well as migrating popcode itself to dayjs.

This PR as-is could be a good approach, too!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@outoftime outoftime

Assignees
No one assigned
Labels
None yet
Projects
Performance
  
In progress
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@razzius @paulproteus @outoftime
You can’t perform that action at this time.