d3 / d3-zoom

Pan and zoom SVG, HTML or Canvas using mouse or touch input.
https://d3js.org/d3-zoom
ISC License
502 stars 143 forks source link

Simplify / clarify rescaleX and rescaleY. #173

Closed altocumulus closed 5 years ago

altocumulus commented 5 years ago

This is inspired by the interesting question Map function in D3, confused about multiple functions passed in which was posted on StackOverflow more than 2 years and ago and is still unresolved. That question asks why at some locations involving d3-zoom a scale object is passed as the this context to a scale's .invert() method like in this Block.

x.domain(s.map(x2.invert, x2));   // 2nd argument; why set this to x2?

As scale.invert() does not make any use of this the call will work just as well if the second argument is omitted. This looked like a small glitch in just one demo at first, interestingly though, it also shows up in the d3-zoom module's own source code, namely, in the methods transform.rescaleX() and transform.rescaleY(). Those functions' bodies are like this:

return x.copy().domain(x.range().map(this.invertX, this).map(x.invert, x));

Right in the last call while passing x.invert as the callback to the .map() method, the x—the scale instance—is passed as the second argument whereby setting the this context. Again, this is not needed nor is it a real error.

Long story short: Is there a deeper reason for explicitly passing in the scale as the this context to the .invert() method or can we get rid of it for the sake of simplicity and clarity?

Fil commented 5 years ago

The reference commit would be https://github.com/d3/d3-zoom/commit/a116a77ebd496495e25070bee6294824a5e2feaf

altocumulus commented 5 years ago

@Fil Any idea on why that was introduced? The commit message just reads "Update README."...

Fil commented 5 years ago

My guess is that some scales' invert function might need to access this, so this would be a proper generic way of calling them.

mbostock commented 5 years ago

The answer is: it’s good practice in JavaScript. If the object’s methods happen to be bound to the object, it’s unnecessary to specify this, but one generally shouldn’t assume so unless it is documented (and not likely to change in a future version).