d3 / d3-scale-chromatic

Sequential, diverging and categorical color scales.
https://d3js.org/d3-scale-chromatic
Other
798 stars 107 forks source link

Mark the package as having no side-effects #20

Closed stof closed 5 years ago

stof commented 5 years ago

This allows for better tree-shaking when using only part of the package when using webpack. By default, it assumes packages have side-effects and so they cannot be skipped by the tree-shaking.

Rollup does not benefit from this (yet) because its equivalent feature is based on a configuration setting treeshake.pureExternalModules which is all-or-nothing.

Here is the effect in my app when I add this in the package.json and analyse the bundle before and after.

Before: d3_scale_chromatic_side_effects

After: d3_scale_chromatic_no_side_effects

In the second image, d3-scale-chromatic is the small column on the right, which contains only colors and categorical/category10, as that's what I use.

stof commented 5 years ago

If this gets accepted, a similar work could be done for most d3 packages d3-transition has side-effects, as it modifies the prototype of d3-selection, but that's the only one I found for now, among the packages I checked (I haven't checked them all yet)

stof commented 5 years ago

Note that Rollup now also benefits from this.

stof commented 5 years ago

@mbostock is there any interest in this ? If yes, I can send PRs to other packages to mark them as side-effect-free when relevant.

yyfearth commented 5 years ago

Any respond to this? I tried if we can make entire d3 side-effect free, with this package, the webpack bundle can save at least 15k after gzip.

stof commented 5 years ago

@yyfearth we cannot make the entire d3 side-effect free, because some packages do have side-effects (d3-transition alters the prototype of selections for instance). But most of d3 can.

mbostock commented 5 years ago

I don’t plan on supporting hints to specific bundlers, so if you want to tell webpack to treat this module specially, you’ll probably need to add some configuration yourself to webpack. My apologies but it’s a pain to support hints and configuration for specific bundlers across all the d3 modules and I have to be conservative with what I support.

stof commented 5 years ago

@mbostock rollup, webpack and parcelJS all agreed on using the same package.json field to mark modules as not having side-effects (and there might be more of them that I don't know of). This does not make it a config for specific bundlers but for most major ones (Google Closure Compiler does not support that, but it does not seem to perform tree-shaking the same way than others either AFAICT, so maybe it does not need it). And the benefits in term of size can be massive when you use only parts of a package (which is quite likely for a package like d3-scale-chromatic, maybe less for d3-selection)

stof commented 5 years ago

and browserify might also support it in the future: https://github.com/browserify/common-shakeify/pull/31

yyfearth commented 5 years ago

@stof Except for the d3-delection and d3-transition, are there any other module that having side effect? I'm try to collect some module names that I can use a postinstall script to patch the package json of them to add the sideEffects: false. Because D3 is huge, by doing this, we can save lots. Esp we are using D3, Highcharts, agGrid, moment and lodash in our react app, the bundle became so big to be loaded (> 1MB after gzip).

stof commented 5 years ago

@yyfearth I haven't checked them all yet. d3-transition is the only package having side-effects among the ones I investigated (d3-color, d3-ease, d3-format, d3-scale, d3-scale-chromatic, d3-selection, d3-shape, d3-transition) Note that d3-selection does not have side-effects. It is affected by side effects of d3-transition

mbostock commented 5 years ago

Related https://github.com/d3/d3/issues/3131. I’m planning on marking everything except d3-transition as sideEffects: false in the next major release.

mbostock commented 5 years ago

Merged as d22e006bab56729dd8f01c440dda9704fff6d243. Thank you!

stof commented 5 years ago

@mbostock why waiting for the next major for other packages while you merged that one in a minor version ? Adding it in a minor version will provide the benefits to everyone now, rather than having to wait for libraries to migrate to d3 v6

stof commented 5 years ago

note that I'm willing to help move forward on this if you want, by sending PRs for a bunch of packages.

mbostock commented 5 years ago

I’m not waiting for the next major release, but I am actively working towards the next major release, and I’m not planning on backporting this to modules that have already received a major version bump (such as d3-scale).