d3 / d3-shape

Graphical primitives for visualization, such as lines and areas.
https://d3js.org/d3-shape
ISC License
2.48k stars 308 forks source link

Error when setting beta in minified version of d3.curveBundle. #97

Closed altocumulus closed 7 years ago

altocumulus commented 7 years ago

There is a question on Stack Overflow reporting a bug in d3.curveBundle: Setting d3.curveBundle.beta seems to have no effect. Setting beta to any value other than the default value does not change the outcome.

As it turns out, the issue shows up only in the minified version of D3 (https://d3js.org/d3.v4.min.js): JSFiddle

The non-minified version works as expected: JSFiddle

Comparing the relevant code parts of the minified vs. the non-minified version, the source of the error becomes apparent:

// non-minified
export default (function custom(beta) {

  function bundle(context) {
    return beta === 1 ? new Basis(context) : new Bundle(context, beta);
  }

  bundle.beta = function(beta) {
    return custom(+beta);
  };

  return bundle;
})(0.85);

This translates to the following minified code:

// minified
var ab = function t(n) {
  function e(t) {
      return new Mc(t, .85)
  }

  return e.beta = function (n) {
      return t(+n)
  }, e
}(.85);

Obviously, these two snippets are not equivalent. In the minified version the parameter n passed to t() is never used internally. Instead, a new instance of Mc—i.e. function Bundle(context, beta)— is created with a fixed value of .85 no matter, what beta was actually set to.

As the original source is correct and works in the non-minified version, I consider the problem to be found somewhere in the built process.

mbostock commented 7 years ago

This is an excessively aggressive optimization by UglifyJS that was introduced in a recent release and seems like a bad default. I will see about disabling it or downgrading UglifyJS.

mbostock commented 7 years ago

This was fixed in uglify-js@2.8.8 (no release notes, regrettably, but possibly it is mishoo/UglifyJS2#1560). I ran a bisection on the following test input:

!function() {
  var foo = (function newFoo(result) {
    function foo() {
      console.log(result);
    }
    foo.result = function(result) {
      return newFoo(result);
    };
    return foo;
  })("fail");
  foo.result("success")();
}();

With 2.8.11, the resulting code is:

!function() {
  (function n(u) {
    function t() {
      console.log(u);
    }
    return t.result = function(u) {
      return n(u);
    }, t;
  })("fail").result("success")();
}();

Which looks fine. So, I think the solution will be to re-release all the D3 modules with an update of UglifyJS, which will be tedious…

mbostock commented 7 years ago

Well, that was extremely tedious. But all D3 modules should now be using UglifyJS 2.8.11 or later, which does not have this bug.