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

#4454 Dynamically import Moment locales to reduce client bundle size #4455

Merged
merged 11 commits into from Aug 2, 2018

Conversation

Projects
None yet
4 participants
@dancastellon
Copy link
Contributor

commented Jul 18, 2018

Resolves #4454
Impact: minor
Type: performance

Issue

When I ran Meteor's bundle visualizer with RC, I noticed that MomentJS (257kb) was included in the initial client bundle, even though some work had previously been done to have it dynamically imported via a Reaction High Order Component. The most significant file was Moment's locales (204kb).

Solution

There was 1 import in /client/modules/core/helpers/templates.js that was loading moment's locales. Moving that line to a dynamic import in lazyLoadMonths() a few lines below, reduces the client bundle size by 204kb.

It is important to note that lazyLoadMonths() is only called in one place - the core monthOptions Meteor template helper. The helper itself is only used in one place - the payments-example plugin.

Note: moment.js still appears in the client bundle, but at a much smaller size (54kb).

Breaking changes

None

Testing

  1. Startup the visualizer with export TOOL_NODE_FLAGS='--max-old-space-size=4096' && METEOR_DISABLE_OPTIMISTIC_CACHING=1 meteor run --production --extra-packages bundle-visualizer
  2. Confirm you don't see moment's locales.min.js in the visualizer. It previously was right after meteor-node-stubs. Note: you will still see moment.js at 54kb. See screenshots attached.
  3. Confirm the overall bundle size is ~4.8mb (down from ~5.8mb)
  4. Load up the app without visualizer: METEOR_DISABLE_OPTIMISTIC_CACHING=1 reaction
  5. Login as admin and enable the example payments plugin, and the Flat Rate Shipping -> Free rate (so you can proceed through checkout)
  6. In the header, switch to another language
  7. In the payment form (last step), confirm the months in the select box appear in the correct language
  8. This confirms 2 things - that the original functionality of payments-example remains, and that dynamic imports still work.
  9. Switch back to English, go back to checkout, and confirm months are now displayed in English in the select box. This confirms the fix for a bug where switching the language didn't update the months in the select box unless you manually refreshed the entire page.
  10. Complete the order
  11. Open the order notifications dropdown (bell icon in header). Confirm the date reads "a few seconds ago". This confirms moment is still working (only dynamic import where syntax was changed)

Before

bundle-moment-locales-before

After

bundle-moment-locales-after

@dancastellon dancastellon requested a review from zenweasel Jul 18, 2018

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@dancastellon I see the testing instructions for how to confirm that the bundle is lighter, but how do I determine that this hasn't broken anything?

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@dancastellon I'm still seeing momentjs_moment listed in the visualizer

@dancastellon dancastellon changed the title #4454 Dynamically import Moment locales to reduce client bundle size WIP #4454 Dynamically import Moment locales to reduce client bundle size Jul 19, 2018

@dancastellon dancastellon changed the title WIP #4454 Dynamically import Moment locales to reduce client bundle size #4454 Dynamically import Moment locales to reduce client bundle size Jul 19, 2018

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Thanks @zenweasel - I updated the PR, description, and testing instructions. I figure this issue & PR can just be for dynamically importing Moment's locales (unless you think otherwise). The rest of core all appears to be importing moment dynamically, yet moment.js (54kb) is still in the bundle as you noted. This is worth more research, as something (maybe a Meteor package) is loading moment.

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

@dancastellon What's the status on this?

Merge branch 'release-1.14.0' of https://github.com/reactioncommerce/…
…reaction into fix-4454-dancastellon-moment-still-included
@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

@zenweasel This is ready for your review. I merged in and tested with the latest 1.14.0. momentjs_locales should be lazy-loaded now (204kb), but momentjs_moment (53kb) is still in the bundle. Should I create a follow-up issue?

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@zenweasel I created a follow-up issue for figuring out the 53kb moment.js that is still included in the bundle - #4479

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

@zenweasel Fixed a couple linting issues here today. This one is now really ready for your review :)

@pmn4

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2018

nice!

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

@dancastellon When following the test instructions (steps 3 & 4) I get this error in the console. It appears there's still a problem here:

modules.js?hash=f0449541cb4a6e748adaa3fda131056e90e0def9:55564 Warning: Failed prop type: Invalid prop `moment` of type `object` supplied to `NotificationRoute`, expected `function`.
    in NotificationRoute (created by Reaction(NotificationRoute))
    in Reaction(NotificationRoute) (created by withProps(Reaction(NotificationRoute)))
    in withProps(Reaction(NotificationRoute)) (created by Tracker(withProps(Reaction(NotificationRoute))))
    in Tracker(withProps(Reaction(NotificationRoute))) (created by Reaction(NotificationDropdown))
    in Reaction(NotificationDropdown) (created by Reaction(Notification))
    in div (created by Reaction(Notification))
    in div (created by Reaction(Notification))
    in Reaction(Notification) (created by withProps(Reaction(Notification)))
    in withProps(Reaction(Notification)) (created by Tracker(withProps(Reaction(Notification))))
    in Tracker(withProps(Reaction(Notification))) (created by Reaction(NavBar))
    in div (created by Reaction(NavBar))
    in div (created by Reaction(TagNav))
    in div (created by Reaction(TagNav))
    in Reaction(TagNav) (created by TagNavContainer)
    in div (created by TagNavContainer)
    in TagNavContainer (created by Tracker(TagNavContainer))
    in Tracker(TagNavContainer) (created by Reaction(NavBar))
    in header (created by Reaction(NavBar))
    in div (created by Reaction(NavBar))
    in Reaction(NavBar) (created by Tracker(Reaction(NavBar)))
    in Tracker(Reaction(NavBar))
    in div (created by Reaction(coreLayout))
    in Reaction(coreLayout) (created by Route)
    in Route
    in Switch (created by Reaction(App))
    in div (created by Reaction(App))
    in div (created by Reaction(App))
    in div (created by Reaction(App))
    in Reaction(App) (created by Tracker(Reaction(App)))
    in Tracker(Reaction(App))
    in TranslationProvider (created by Tracker(TranslationProvider))
    in Tracker(TranslationProvider)
    in Router (created by BrowserRouter)
@zenweasel
Copy link
Member

left a comment

Throwing an error in the console when launching the app

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

@zenweasel Fixed - I now see why we need dynamic-import-node :)

@dancastellon dancastellon requested a review from zenweasel Jul 25, 2018

@dancastellon dancastellon changed the title #4454 Dynamically import Moment locales to reduce client bundle size WIP #4454 Dynamically import Moment locales to reduce client bundle size Jul 26, 2018

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@zenweasel As I mentioned in our sync call, removing the babel dynamic import module and adjusting the syntax of the dynamic imports made the bundle about 1mb smaller. Pushed the change, but moved this back to WIP while I test.

@dancastellon dancastellon changed the title WIP #4454 Dynamically import Moment locales to reduce client bundle size #4454 Dynamically import Moment locales to reduce client bundle size Jul 27, 2018

@dancastellon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

@zenweasel This is now ready for your review. I ended up only having to change dynamic import syntax for moment, which was pretty easy to confirm (see updated testing instructions)

@zenweasel
Copy link
Member

left a comment

Tested. Verified works well. :shipit:

@zenweasel

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

@aldeed Sorry, I approved this yesterday but I guess I also needed to dismiss my review. If we could merge this one as well I think thats everything for release-1.14.0

@aldeed aldeed merged commit 11677a3 into release-1.14.0 Aug 2, 2018

10 of 11 checks passed

License Compliance 6 issues found
Details
WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: snyk-security Your tests passed on CircleCI!
Details
ci/circleci: test-app Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk - package.json (Reaction Commerce) No new issues
Details

@aldeed aldeed deleted the fix-4454-dancastellon-moment-still-included branch Aug 2, 2018

@spencern spencern referenced this pull request Aug 8, 2018

Merged

Release 1.14.0 #4338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.