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 sideEffects: false to package.json. #3131

Closed
29 of 33 tasks
TheLarkInn opened this issue Aug 8, 2017 · 35 comments
Closed
29 of 33 tasks

Add sideEffects: false to package.json. #3131

TheLarkInn opened this issue Aug 8, 2017 · 35 comments

Comments

@TheLarkInn
Copy link

TheLarkInn commented Aug 8, 2017

Hello D3 Family!!!!! @TheLarkInn from webpack here!

We have been working on a new feature for webpack 4+ which allow's library authors to add a special field to package.json allowing them to declare that their libraries that have "reexports" in them do not contain side effects!

Tiny Backstory

Because webpack implements the Harmony Module Specification to spec, we include code that was reexported (even if it wasn't used). This is because all code [per spec] must be eval'd). The beautiful news is that we do not have to implement exactly to spec. So we have created what is called "pure-module". webpack as imports are being resolved will also check to see if the module is pure (which is indicated through the pure-module flag in the package.json fields). Therefore, there would be no bundle size difference between the following examples:

import {bottomAxis} from "d3-axis";

and

import bottomAxis from "d3-axis/direct/path/to/d3AxisBottom"

Proposal

If there are no side-effects within the D3 packages in places where re-exports occur, then I'd love to propose that all D3 packages add pure-module: true to their package.json.

Pros

Developers (using webpack) get huge perf wins by doing nothing.

Cons

Bit more work to get it all done.

Please let me know if you have any questions, the webpack team is really anxious to help in any way if we can (that means, submitting PR's, adding issues to individual repo's to track, etc).

sideEffects: true

sideEffects: false

@mbostock
Copy link
Member

mbostock commented Aug 8, 2017

Great! Thanks for the request.

How can we test that it is working as intended? Most D3 modules should be side-effect free, but I know of at least a few places that are not, and I don’t know whether there is a way to restructure them to make them “pure”. Some examples:

I’m not sure if there are others. My guess is that the rest should be considered pure but I don’t know how to verify that.

@curran
Copy link
Contributor

curran commented Aug 8, 2017

This is awesome! So glad to see some ideas from the original discussion in #3076 are coming to fruition.

@TheLarkInn
Copy link
Author

TheLarkInn commented Aug 8, 2017

How can we test that it is working as intended?

For now you could pull down the PR to test it out. (In the future we are working on webpack-canary which can help us and everyone else test webpack versions/commits/branches against anything they want.

Currently today the behavior is this (I'm using a plugin in VSCode that calculates the cost based on way it is imported):

Larger sized because reexported code is being evaluated and included in bundle even if not used
d3importlarge

Smaller sized because importing directly from module w/ no reexports
d3importsmall

So with this pure-module change, you will know its working in that you can use either syntax seen above and the size should be representative of the second image. (aka it should be small no matter what for any pure modules).

Most D3 modules should be side-effect free, but I know of at least a few places that are not, and I don’t know whether there is a way to restructure them to make them “pure”.

I think there are going to be times where its not really avoidable and that's totally fine. If that's the case you can leave the property off for those specific modules and I think that's totally fine. In terms of suggestions to do it another way, I'm not quite sure what would be the easiest approach or if even possible.

Once webpack-canary is closer to a place where it could be leveraged in lets say unit/functional tests, I'll let you know, but otherwise if you wanted to try it out earlier you can try and run webpack from this PR.

Let me know if there's any other ways we can help. When I find some time in the near future, I'll try and run the PR locally against a modded package.json file for d3 and make sure it is doing the expected behaviors myself. Thank you for the collaboration. This is going to be an awesome out of the box perf win when people use d3 with webpack 4 and that's what excites me the most.

@martgnz
Copy link

martgnz commented Oct 28, 2017

@TheLarkInn, any news on this?

@erosenberg
Copy link

erosenberg commented Nov 29, 2017

Not sure if related, but I'm getting the following error from ESLint because of the way that these modules are exported/imported:

[eslint] 'd3-scale' should be listed in the project's dependencies. Run 'npm i -S d3-scale' to add it (import/no-extraneous-dependencies)

Edit: I should mention I actually have d3 v4 in my package.json along with vx but not each module individually.

@mbostock
Copy link
Member

@erosenberg They are listed as dependencies; see the package.json.

@erosenberg
Copy link

erosenberg commented Nov 29, 2017

@mbostock - thanks for your speedy reply! I believe it meant my project's package.json and not d3's. Even still, ESLint should recognize that it's a dependency of a dependency, so that may just be an ESLint issue.

@TheLarkInn
Copy link
Author

I'll be giving more of an update as webpack 4 draws near. Thanks for all the patience folks.

@martgnz
Copy link

martgnz commented Feb 26, 2018

@TheLarkInn Any news on this now that v4 is out?

@marcofugaro
Copy link

Nice! So when d3 adds sideEffects: false to its package.json, it will allow us to do

import { scaleLinear } from 'd3'

without thinking about it?

@TheLarkInn
Copy link
Author

Yess that correct. So the change can come by adding sideEffects: false today at the package.json. I know there are some exports which do prototype things, however the only sideEffects we are concerned about are the ones that cause other exports within a module to be affected

@TheLarkInn
Copy link
Author

TheLarkInn commented Mar 17, 2018 via email

@tomwanzek
Copy link

@TheLarkInn To continue your clarification, what about inter-package side-effects? Specifically for D3:
d3-transition and d3-selection-multi have side-effects on d3-selection by extending the prototype of one of its core exports, Selection.

According to your last comment, these side-effects would not require a sideEffects: true statement in and of themselves. Could you kindly confirm this understanding?

@marcofugaro
Copy link

Wait, does it also works if I put sideEffects: false to MY project's package.json? (where I import d3)

Or do we have to wait for d3 to publish its updated package.json?

@TheLarkInn
Copy link
Author

Here is a better explanation that I provided for a user on Stack Overflow. Please let me know if this answers your questions.

https://stackoverflow.com/a/49203452/1624684

@TheLarkInn
Copy link
Author

@marcofugaro it will only be specific to your code and not other node modules.

@TheLarkInn TheLarkInn changed the title [RFC]: Add pure-module: true to package.json for D3 packages. [RFC]: Add sideEffects: false to package.json for D3 packages. Mar 19, 2018
@TheLarkInn
Copy link
Author

With this information in mind would it be safe to say we can add this flag in the top d3 package?

@mbostock
Copy link
Member

@TheLarkInn I haven’t done this because I haven’t had time to find where all the side-effects are in D3. I identified a few likely cases above #3131 (comment), but there could be more, and I’m not sure how I would easily discover them. I don’t feel safe turning this on without someone surveying the code and making sure it won’t break users.

@TheLarkInn
Copy link
Author

Totally understandable and no worries. Just wanted to double-check!! Keep up the killer work.

@mbostock mbostock mentioned this issue Jul 31, 2019
29 tasks
@mbostock
Copy link
Member

mbostock commented Aug 1, 2019

If I understand the design of the sideEffects flag correctly, we can’t set sideEffects: false on the top-level d3 package because this package imports both d3-selection and d3-transition, and d3-transition modifies the d3.selection.prototype. So, if we set sideEffects: false, if you

import {select, selectAll} from "d3";

you’ll no longer be able to call selectAll(…).transition() because webpack will not include the import of d3-transition which applies the side-effect.

That said, I don’t understand how the sideEffects flag applies recursively to packages. If d3 has sideEffects: true, d3-transition has sideEffects: true, and (say) d3-scale and d3-selection have sideEffects: false, will webpack be smart enough not to pull in d3-scale in the above case?

@mbostock
Copy link
Member

mbostock commented Aug 1, 2019

Per #3131 (comment), this is my best guess as to which D3 modules have side-effects according the definition above.

My current plan is not to test whether these flags are correct (because it would be a lot of work), but to release them and see if people complain. If someone who uses webpack wants to volunteer to verify these flags and submit PRs to every D3 module, that’d be fantastic.

@mbostock
Copy link
Member

mbostock commented Aug 1, 2019

Upon further reflection, I don’t think d3-format and d3-time-format are considered side-effects according to the description above, since the side-effect in src/defaultLocale.js is simply to set the values of the bindings that are exported by that module; it’s not modifying anything exported by another module.

So it really is just d3-transition that has the side-effect of modifying d3.selection.prototype.

@mbostock mbostock changed the title [RFC]: Add sideEffects: false to package.json for D3 packages. Add sideEffects: false to package.json. Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

15 participants