d3 / d3-geo-polygon

Clipping and geometric operations for spherical polygons.
https://d3.observablehq.cloud/d3-geo-polygon/
Other
113 stars 23 forks source link

Mark the package as having no side effects #32

Closed stof closed 5 years ago

Fil commented 5 years ago

this package rewrites a few projections from d3-geo-projection ; isn't that considered a side effect?

stof commented 5 years ago

this package does not depend on d3-geo-projection at all, so I don't see how it could affect it.

stof commented 5 years ago

Clashes in the global d3 variable when loading the UMD build are out of scope for this: this flag is relevant when using a module bundler, for which there won't be such clashes.

Fil commented 5 years ago

It defines alternative versions of 4 symbols: https://github.com/d3/d3-geo-polygon/blob/master/src/index.js#L3 vs https://github.com/d3/d3-geo-projection/blob/master/src/index.js#L70

stof commented 5 years ago

External side effects are mostly 2 things:

A third way to have external side effects is to import another module having such side effects (and s d3-zoom and d3-brush are not side-effects-free because they import d3-transition)

Fil commented 5 years ago

OK thanks for the explanation, now it makes sense to me. Suspending for now: I'm trying to adhere as much as possible to the other d3 packages, so I'll follow whatever happens there.

mbostock commented 5 years ago

We’re adding this to all other D3 modules except d3-transition and d3-selection-multi.

Fil commented 5 years ago

thank you!

Fil commented 5 years ago

I made a small project to see for myself what the gains are: https://github.com/Fil/sideEffects-test-d3-geo

sharing it because it might interest someone else (note: it's my first try with webpack)

stof commented 5 years ago

@mbostock packages which import d3-transition actually also trigger side-effects due to d3-transition. I actually opened PRs for all d3 packages which can have it.

mbostock commented 5 years ago

@stof Referring to the dependency diagram: chart

Only d3, d3-brush and d3-zoom import d3-transition: d3-zoom initiates a transition on doubleclick, and d3-brush supports transitions, and thus must interrupt a transition at the start of a brush gesture. But is it necessary to declare that d3-brush and d3-zoom have sideEffects? Even if the side-effect is that d3.selection.prototype is modified to support transition methods, I don’t see that importing d3-brush or d3-zoom should guarantee that this happens. Both d3-brush and d3-zoom could work without transitions.

stof commented 5 years ago

@mbostock sure, they would work. But tree-shaking them would mean that code elsewhere using d3-selection would no longer have access to .transition() on them if they forget to import d3-transition themselves (because they missed the fact that there is a side effect here).

A solution here would be to provide a side-effects free entry point in d3-transition (separate from index.js which has a side-effect when loading it), which would export the transition API without registering the selection prototype method, allowing some packages to integrate with d3-transition in a side-effects-free way. Then, the next major version of d3-zoom and d3-brush could switch to this entry point and become side-effect free (and d3-transition could be updated to declare that only index.js and selection/* have side effects). Another solution is to split selection.prototype.transition and selection.prototype.transition into a separate (side-effect-full) package in the next major version, that would be imported when you want the transition shortcut on selections (which is not really necessary when wanting to interrupt btw).

stof commented 5 years ago

note that d3-zoom actually uses selection.prototype.transition currently, so it relies on the side-effect-full API currently. d3-brush only relies on a side-effect-free API so would benefit from my previous suggestion.