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

[email protected] sideEffects:false breaks graph axis formatting #84

Closed
dpmott opened this issue Dec 20, 2019 · 7 comments
Closed

[email protected] sideEffects:false breaks graph axis formatting #84

dpmott opened this issue Dec 20, 2019 · 7 comments

Comments

@dpmott
Copy link

dpmott commented Dec 20, 2019

[email protected] sideEffects:false breaks graph axis formatting

The d3-format src\defaultLocale.js calls defaultLocale at load time. This is, by definition, a side effect of loading the module. As such, it's inappropriate to set sideEffects:false in the package.json file. I believe that this is the root cause of this issue.

For our project compiled for production, [email protected] causes d3-scale\src\tickFormat.js to exception when it calls format(), because format has not been defined, presumably because webpack's tree shaking has entirely removed d3-format's invocation of defaultLocale().

The docs at https://www.npmjs.com/package/d3-format say this:

If you do not set a default locale, it defaults to U.S. English.

However, this is not true when compiling in production mode with [email protected].

When I explicitly install [email protected], or if I explicitly call d3d3.formatDefaultLocale(), then I do not get an exception, and my graphs render in my application.

@mbostock
Copy link
Member

My understanding is that sideEffects: false is intended to mean there are no side-effects that cross modules, not that there are no side-effects within the module. That d3-format has an internal side-effect does not violate the intent of “sideEffects: false”.

What are you using to compile your code?

@dpmott
Copy link
Author

dpmott commented Dec 20, 2019

We're using stock Angular 7.2.15, typescript 3.2.4, node v10.16.3, npm 6.9.0.

The failure comes in this (pseudo)code:

private svg: d3.Selection<SVGGElement, number, HTMLElement, number>;
private xScale: d3.ScaleLinear<number, number>;
private xAxis: d3.Axis<number | { valueOf(): number }>;
protected selector: string; // set to something like '#my-id .my-chart'

this.svg = d3.select(this.selector)
      .append('svg')
      .attr('width', ...)
      .attr('height', ...)
      .append('g')
      .attr('transform', `translate(0, ...)`);

this.xScale = d3.scaleLinear()...

this.xAxis = d3.axisBottom(this.xScale)
      .ticks(...)
      .tickValues(...)
      .tickSize(...);

this.svg.append('g')
      .attr('class', 'x-axis')
      .attr('transform', `translate(0, ...)`)
      .call(this.xAxis) // <--- this exceptions in a production build

The error is:

TypeError: (void 0) is not a function
    at tn (vendor.ed9171441c8bdb36a768.js:1)
    at Function.e.tickFormat (vendor.ed9171441c8bdb36a768.js:1)
    at d (vendor.ed9171441c8bdb36a768.js:1)
    at C.call (vendor.ed9171441c8bdb36a768.js:1)
    at t.drawXAxis (main.6d696a7192e551f1463d.js:1)
    at t.render (main.6d696a7192e551f1463d.js:1)
    at n.ngAfterViewChecked (main.6d696a7192e551f1463d.js:1)
    at Xi (vendor.ed9171441c8bdb36a768.js:1)
    at $i (vendor.ed9171441c8bdb36a768.js:1)
    at Qi (vendor.ed9171441c8bdb36a768.js:1)

I instrumented various functions in the d3 ecosystem, and determined that defaultLocale() in d3-format src\defaultLocale.js was never getting called in a production build. When I added a call from our code to d3.formatDefaultLocale() (which is the exported name for d3-format's defaultLocale()), the above call to .call(this.xAxis) completed without error.

The only production change that I see between d3-format v1.4.1 and v1.4.2 is the addition of the sideEffects:false flag in package.json. 🤷‍♂

@dpmott
Copy link
Author

dpmott commented Dec 20, 2019

Here's a link to the webpack docs that may help:

https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

To quote:

A "side effect" is defined as code that performs a special behavior when imported, other than exposing one or more exports. An example of this are polyfills, which affect the global scope and usually do not provide an export.

I would think that some global variable initialization would meet this definition of "side effect".

@mbostock
Copy link
Member

mbostock commented Dec 20, 2019

Angular’s optimizer is known to break code, and they don’t appear to have plans to fix this behavior. This has affected other D3 libraries (e.g., d3/d3-color#68 (comment)), and may be the cause of what’s happening here.

This change to add sideEffects: false was suggested by a Webpack maintainer; see d3/d3#3131. I don’t personally use Webpack, and I find the documentation to be somewhat ambiguous (does a “module” mean any individual file, as in ES modules, or do they mean the package in total, as in npm & node_modules?). I don’t know how to determine what is appropriate here.

From my perspective, the only thing adding sideEffects: false has brought is an influx of issues from Angular users. (Sorry for the gripe!)

@mbostock
Copy link
Member

Proposed fix in #85. I think we’ll need a similar fix for d3-time-format.

@mbostock
Copy link
Member

No response, but I merged #85, so I’ll publish that and see what happens.

@dpmott
Copy link
Author

dpmott commented Jan 2, 2020

Thanks for the quick resolution on this, and Happy New Year! ❤️

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

Successfully merging a pull request may close this issue.

2 participants