-
Notifications
You must be signed in to change notification settings - Fork 57
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: output mjs modules to enable tree shaking #395
Merged
Merged
Conversation
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
🚀 Preview deployed Built with commit 9bae595 https://deploy-preview-395--stoic-hodgkin-c0179e.netlify.com |
aldeed
force-pushed
the
feat-aldeed-modules
branch
from
February 6, 2019 20:56
c97352f
to
4117eb1
Compare
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
In Jest 24+, the "default" reporter is disabled when reporters are in config, unless we explicitly add it Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
aldeed
force-pushed
the
feat-aldeed-modules
branch
from
February 6, 2019 20:57
4117eb1
to
9bae595
Compare
aldeed
changed the title
feat: output mjs modules
feat: output mjs modules to enable tree shaking
Feb 6, 2019
willopez
approved these changes
Feb 8, 2019
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.
Ran the storefront with the build library and it runs correctly 👍
🎉 This PR is included in version 0.63.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Impact: minor
Type: feature
Changes
Updates to enable tree shaking in Webpack:
main
toindex
with no extension. Node will then pick either.mjs
or.js
based on which flags it is run with (if anything is imported from this package in a Node app, which is unlikely).module: "index.mjs"
inpackage.json
, which Webpack prefers overmain
if it's there.sideEffects: false
in thepackage.json
We are including both a
.js
entry point that is commonjs and a.mjs
entry point that uses ES modules. Tools like Webpack know how to figure out which to use.One goal here is that if you don't import components that need certain dependencies, then you won't need to install those dependencies. Particularly, if you are not using the Stripe components, you should not need to install the Stripe-related peer dependency packages.
Another goal is just to eliminate unused code from the production bundle of a Webpack app. Unfortunately, this doesn't seem to fully work in the NextJS example storefront yet. It sounds like it may work when NextJS 8 is released.
This PR also fixes an issue introduced by the earlier update to Babel 7 and Jest 24. Jest 24 no longer keeps the default reporter if there is a
reporters
config, which this repo has. This meant that when you ran tests locally, it would appear to hang and pass or fail, but without actually logging any of the normal output. I added "default" reporter explicitly and things are back to normal. This was a bug affecting running tests only.Breaking changes
None. We are including both a
.js
entry point that is commonjs and a.mjs
entry point that uses ES modules. Tools like Webpack know how to figure out which to use.Testing
Just verify that the example storefront works when you link in the updated package build.
In the components library repo, not in Docker:
Then check out this branch of the NextJS storefront: reactioncommerce/example-storefront#495. Stop the NextJS storefront and make the following change in its
docker-compose.yml
file:volumes: - $HOME/.cache/yarn-offline-mirror:/home/node/.cache/yarn-offline-mirror - web-yarn:/home/node/.cache/yarn - .:/usr/local/src/reaction-app - empty_node_modules:/usr/local/src/reaction-app/node_modules # do not link node_modules in, and persist it between dc up runs - node_modules:/usr/local/src/node_modules + - /Users/eric/projects/reaction-component-library/package/dist:/usr/local/src/node_modules/@reactioncommerce/components
The part before the colon in that added line must be the exact full path to the
/package/dist
folder of this repo on your machine, the one that was created when you ranyarn run build
above. This will link the build from the checkout into thenode_modules
that is used in the Docker container.Then run
docker-compose up -d
like normal to access the app in your browser and test it. Be sure to remove and not commit the added line fromdocker-compose.yml
when you are done testing.