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

Add module support for tree shaking #143

Closed
dylanmoz opened this issue Sep 10, 2017 · 13 comments · Fixed by #298
Closed

Add module support for tree shaking #143

dylanmoz opened this issue Sep 10, 2017 · 13 comments · Fixed by #298

Comments

@dylanmoz
Copy link
Contributor

Add support for the "module" field in package.json so that webpack/rollup can do proper tree shaking. If you use @vx currently with Rollup, you will get an error by default due to the only availability being commonjs that it cannot understand, so you have to add all the exports of each sub-library of @vx explicitly in rollup.config.js:

https://github.com/rollup/rollup-plugin-commonjs#custom-named-exports

The result is that if you import one part of a @vx/shape, for example, you have to import its entirety.

I believe this can be as simple as building each library twice: Once as it currently is (default es5 transpiling by babel), and a second build into another folder, such as module, where the modules flag is turned off for babel in the babelrc:

"presets": [["es2015", { "modules": false }], "react", "stage-0"],

https://babeljs.io/docs/plugins/preset-es2015/#options

@hshoff
Copy link
Member

hshoff commented Sep 10, 2017

Ah yes, i've been putting off getting this going. I'll take a swing at this soon. Will definitely have it sorted out by v1.0.0.

for now you should be able to use absolute paths:

import LinePath from '@vx/shape/build/shapes/LinePath';

@ljharb
Copy link

ljharb commented Sep 12, 2017

The "module" field is nonstandard and should not be used.

"Proper tree-shaking" should not, and does not, require ES Modules; it can work equally well with (static, top-level) CJS - although popular bundlers may not have chosen to build support for that yet.

Using deep paths is always the best practice.

Separately, if you want to follow the actual thing that node will be doing (which is what the ecosystem will also end up doing), you should generate .mjs files alongside all your .js files, and the .mjs files need to use only syntax that's in node v8.5 (or whatever version ends up first landing unflagged ESM support).

@techniq
Copy link
Collaborator

techniq commented Dec 21, 2017

We might also want to look into adding pure-module: true to our modules for Webpack 4. See d3 issue for details

@ljharb
Copy link

ljharb commented Dec 22, 2017

@techniq i believe it's now sideEffects: false.

@hshoff
Copy link
Member

hshoff commented Jan 11, 2018

@knoopx
Copy link

knoopx commented Feb 20, 2018

Three shaking won't work using "deep paths" references to individual component modules on the app side unless the library also uses "deep paths" within the library code.

@ljharb
Copy link

ljharb commented Feb 20, 2018

@knoopx tree-shaking isn’t needed when using deep paths (absolute paths - starting with / - are never a good idea)

@knoopx
Copy link

knoopx commented Feb 22, 2018

Yes I was referring to "deep paths". The library should use deep paths too in order to be able to use "deep paths" on the app side, otherwise you get everything bundled even when you are only importing a single module.

@knoopx
Copy link

knoopx commented Feb 22, 2018

For example I'm using only these modules:

import AxisBottom from '@vx/axis/build/axis/AxisBottom'
import AxisLeft from '@vx/axis/build/axis/AxisLeft'
import Group from '@vx/group/build/Group'
import Line from '@vx/shape/build/shapes/Line'
import LinePath from '@vx/shape/build/shapes/LinePath'
import Point from '@vx/point/build/Point'
import scaleLinear from '@vx/scale/build/scales/linear'
import ScaleSVG from '@vx/responsive/build/components/ScaleSVG'

However my bundle contains a bunch more than the ones I'm actually using:

image

And that happens because the library itself is not using "deep paths", nor proper "three shaking" to import modules it depends on:

https://github.com/hshoff/vx/blob/master/packages/vx-shape/src/shapes/LinePath.js#L5

@hshoff
Copy link
Member

hshoff commented Feb 22, 2018

That’s my fault. Will get that fixed. Thanks @knoopx

@knoopx
Copy link

knoopx commented Feb 22, 2018

@hshoff I personally think it would be easier to distribute vx also as a module and let the "tree shaking" do the hard work as this issue suggest. Anyway thanks for taking the "deep path" feedback into account.

@hshoff
Copy link
Member

hshoff commented Feb 22, 2018

Yup, absolutely. I need to update the build tooling.

@ljharb
Copy link

ljharb commented Feb 22, 2018

For me, “tree-shaking helps” is an indication that architecture is sub-par, and that something should be improved.

It seems like if internal usage in vx changes to use deep paths instead of using “manifest exports” - ie, a file that imports a bunch of things and re-exports them - that the bundle size issues will be addressed.

hshoff added a commit that referenced this issue Jun 5, 2018
[new build] use rollup, add es + umd builds. fixes #143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants