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

fix(treeshaking): fix treeshaking and add esm and umd versions #1036

Closed
wants to merge 2 commits into from

Conversation

davidicus
Copy link
Collaborator

Closes #993 closes #1011

Summary

Changes to our build to allow for es and umd versions and to allow for treeshaking in consuming applications.

Change List (commits, features, bugs, etc)

  • items_here

Acceptance Test (how to verify the PR)

yarn build should output a lib, es, umd, and scss folder.

@netlify
Copy link

netlify bot commented Mar 24, 2020

Deploy preview for carbon-addons-iot-react ready!

Built with commit 26ff0ca

https://deploy-preview-1036--carbon-addons-iot-react.netlify.com

@scottdickerson
Copy link
Contributor

@davidicus so the sideEffects false really was required for treeshaking to work? Sorry for the bad info about that?

@tay1orjones tay1orjones changed the title fix(treehsaking): fix treehsaking and adad esm and umd versions fix(treehsaking): fix treehsaking and add esm and umd versions Mar 25, 2020
@tay1orjones
Copy link
Member

tay1orjones commented Mar 25, 2020

@scottdickerson Yep, here's an example from webpack

From the link: (I replaced the example package name with ours)

The "sideEffects": false flag in carbon-addons-iot-react package.json indicates that the package's modules have no side effects (on evaluation) and only expose exports. This allows tools like webpack to optimize re-exports. In the case

import { a, b } from "carbon-addons-iot-react"

is rewritten to

import { a } from "carbon-addons-iot-react/a"; 
import { b } from "carbon-addons-iot-react/b";

Copy link

@sstone2423 sstone2423 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outputs all 4 directories as intended on yarn build

@@ -61,36 +23,6 @@ Map {
"kind": "primary",
"loading": false,
},
"propTypes": Object {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing most (if not all) propTypes removed from the api snapshot. Is this intentional? I thought the purpose of this file was to compare the APIs from build to build, release to release, etc.

Copy link
Member

@tay1orjones tay1orjones Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes part of this is intended, but we shouldn't be stripping them completely. They should be wrapped in a conditional (see my review), so consuming applications still get proptype warnings.

The broader scope here is that there is a bug with Terser, with a decent workaround synopsis here, where tree shaking breaks when 2 or more static properties are assigned to a function. Our components typically have defaultProps and propTypes. Carbon ran into the same issue recently causing them to revert an update to their build.

We can avoid this by wrapping prop types in a conditional so production builds have only one static property on the component, which allows tree shaking to not break.

@stuckless
Copy link
Contributor

I have a couple questions regarding this, but these questions should not impact the approval of the PR.

  1. Have you validated what the savings are? ie, have you tried creating a single app and include, say, Button only in your app, and compared what the before/after sizes are for the entire build/ output tree. ie, I'd be curious to know a concrete savings number.
  2. What is required from a downstream consumer to enable this?
  3. I assume if a downstream consumer does nothing, then there is no impact to their build.
  4. The webpack example is pretty lean with index.js and files all in the same directory. Does this work if your src repo has sub directories...ie, does webpack convert import {a} from 'module' to something like import {a} from 'module/component/a' if a is within some other directory?
  5. Does enabling this speed up the consuming build or slow it down... or no difference?

@sstone2423 sstone2423 changed the title fix(treehsaking): fix treehsaking and add esm and umd versions fix(treehsaking): fix treeshaking and add esm and umd versions Mar 26, 2020
@sstone2423 sstone2423 changed the title fix(treehsaking): fix treeshaking and add esm and umd versions fix(treeshaking): fix treeshaking and add esm and umd versions Mar 26, 2020
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to these changes, could the documentation in the root README.MD be modified to specify the new scss/styles.scss entrypoint vs the one in lib/scss/styles.scss?

babel.config.js Show resolved Hide resolved
babel.config.js Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@tay1orjones
Copy link
Member

tay1orjones commented Mar 26, 2020

@stuckless I'll step in and answer as I've occasionally been pairing with @davidicus on this for the past week or so.

  1. Have you validated what the savings are? ie, have you tried creating a single app and include, say, Button only in your app, and compared what the before/after sizes are for the entire build/ output tree. ie, I'd be curious to know a concrete savings number.

Yes, originally the entire library was included no matter which components were imported. About ~2.5mb as I recall. After testing in a create-iot-react-app @davidicus saw this drop to ~200k with the bare boilerplate generated by create-iot-react-app (mainly just some UIShell components). I think he has some screencaps he could share to show exacts. Bundle size savings for applications will depend on how many components are imported/used.

  1. What is required from a downstream consumer to enable this?

To enable with Webpack as your module bundler: (below from the link)

  • Use ES2015 module syntax (i.e. import and export).
  • Ensure no compilers transform your ES2015 module syntax into CommonJS modules (this is the default behavior of the popular Babel preset @babel/preset-env - see the documentation for more details).
  • Add a "sideEffects" property to your project's package.json file.
  • Use the production mode configuration option to enable various optimizations including minification and tree shaking.
  1. I assume if a downstream consumer does nothing, then there is no impact to their build.

Yep, shouldn't be any change. Do note that webpack will likely automatically begin importing the esm version by default instead of the lib version because of the addition of "module": "es/src/index.js" in package.json. A bit more info on this from webpack here

  1. The webpack example is pretty lean with index.js and files all in the same directory. Does this work if your src repo has sub directories...ie, does webpack convert import {a} from 'module' to something like import {a} from 'module/component/a' if a is within some other directory?

Yes. It relies on the static structure of ES2015 module syntax resolving relative and named imports

  1. Does enabling this speed up the consuming build or slow it down... or no difference?

Likely no difference to your app's build times, but If your app only uses a small subset of components, tree shaking will provide smaller bundle(s) delivered in production environments.

@stuckless
Copy link
Contributor

@tay1orjones thx... that great info. btw, is Carbon doing this as well?

@tay1orjones
Copy link
Member

@stuckless Yes, Carbon provides an esm build that supports tree shaking.

The bug referenced in my comment above was when they tried to ship a single entrypoint generated by Rollup for their ESM and CJS bundles.

@davidicus
Copy link
Collaborator Author

closing in favor of #1045. Will address all comments in other PR.

@tay1orjones tay1orjones deleted the build-scripts branch October 22, 2020 10:53
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.

Tree shaking is not working in the PAL addons repo Is tree-shaking working?
5 participants