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. #3328

Closed
wants to merge 1 commit into from

Conversation

jomaxx
Copy link

@jomaxx jomaxx commented Jan 16, 2019

@mbostock
Copy link
Member

Does this flag apply only to the source code in this repository (i.e., index.js), or does it apply recursively to the D3 dependencies?

If it only affect webpack’s consumption of this repository and not transitive dependencies, then I would be happy to merge this.

If it affects consumption of dependencies, then the problem (as discussed in earlier issues) is that some D3 code has side-effects. For example, d3-transition adds selection.interrupt and selection.transition to the Selection prototype:

https://github.com/d3/d3-transition/blob/master/src/selection/index.js

I’m therefore hesitant to add this flag because it’s likely to cause breakages (if this flag affects dependencies).

@jomaxx
Copy link
Author

jomaxx commented Jan 16, 2019

@mbostock doesn't apply to transitive dependencies. You can also apply it in a sub-directory of a package if you add a package.json to that folder.

@jomaxx
Copy link
Author

jomaxx commented Jan 16, 2019

might be worth considering adding this optimization to some of the other d3 packages this package is re-exporting

@jomaxx
Copy link
Author

jomaxx commented Jan 17, 2019

@mbostock looking at it again. you are right it would cause issues.

import { select } from 'd3';

would be the equivalent of

import { select } from 'd3-selection';

which would not apply the changes fromd3-transition.

How do users handle this today? The README does say first that you can import directly from specific d3 modules.

@mbostock
Copy link
Member

If you want transition methods, you need to import d3-transition explicitly. E.g.,

import {select} from "d3-selection";
import "d3-transition";

@jomaxx
Copy link
Author

jomaxx commented Jan 17, 2019

I see. Then I don’t think this change is right.

It would be a breaking change since the behavior of importing select from d3 would be different.

In the future would d3 consider removing these side effects?

@mbostock mbostock changed the title Update package.json Add sideEffects: false to package.json. Jan 18, 2019
@mbostock
Copy link
Member

Removing this side effect would require folding d3-transition into d3-selection, which would make d3-selection’s dependency graph much bigger:

d3-selection d3-transition

Also, this is only an example, albeit a pretty stiff one because the side effect is necessary by design. There are other side effects that could be avoided by using lazy initialization. For example, d3-time-format initializes the default locale as a side effect:

https://github.com/d3/d3-time-format/blob/master/src/defaultLocale.js

@jomaxx
Copy link
Author

jomaxx commented Jan 19, 2019

@mbostock understood. to support this, there would need to be architecture changes and i'm not too familiar with the internals of d3 to understand how each module is coupled together.

@mbostock mbostock closed this Jan 19, 2019
@jstcki
Copy link

jstcki commented Jan 22, 2019

@mbostock Is there a chance that you'll reconsider this? I tried it out and the savings can be quite significant, e.g. for d3-scale. Webpack and Parcel both support the sideEffects property, and Rollup is considering about adding Plugin support for it.

Note that there isn't only sideEffects: false but also a way to list files which contain side effects (e.g. like sideEffects: ["./foo.js", "./bar.js"], which could be used in d3-transition, d3-time-format etc.

As a sidenote, d3-transition mutating d3.selection has been one of the most weird bugs to track down for me (which stops working when you upgrade packages and you end up with two versions of d3-selection in the dependency tree). I wonder why d3-transition not just re-exports a different version of d3-selection altogether and one could either use d3-selection or d3-transition in their code but not both together. For example:

-import {select} from "d3-selection";
-import "d3-transition";
+import {select} from "d3-transition";

I'd be happy to help out here with more investigation or pull requests 😃

@stof
Copy link

stof commented Apr 30, 2019

@herrstucki this issue for d3-transition is related to d3/d3-transition#92

Regarding this flag, I opened a PR on d3/d3-scale-chromatic#20 to add it to d3-scale-chromatic, which indeed has no side-effect. I think we could start adding it to all packages which are side-effect-free.
I'm not even sure the main d3 package needs anything here, as it re-exports the other packages (well, it might benefit from it if all of d3 was side-effect-free, but that's not the case)

@stof
Copy link

stof commented Apr 30, 2019

btw, the time-format thing is not a side-effect. This initialization is an effect only inside the module. It is not a side-effect that would have a meaning even when you ignore the exported API of the module.
See https://stackoverflow.com/questions/49160752/what-does-webpack-4-expect-from-a-package-with-sideeffects-false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants