Open jameshadfield opened 1 week ago
Thanks for the detailed write-up. I haven't fully wrapped my head around the fat-arrow function stuff, but I've opened PR #1913 to align the type versions.
Thanks for the detailed write-up. I haven't fully wrapped my head around the fat-arrow function stuff
MDN has a good explanation of the "don't bind this
" thing — there are times when this is actually useful, and then there are times when it turns around and bites you…
I encountered this while fixing a specific bug in d3, but I suspect the problem is much more widespread within Auspice rather than just that described below, hence the broad issue title.
background
The d3-selection library we use to bind event listeners is version
1.4.2
. Viewing past API docs for d3 libraries is difficult but the relevant docs (which match my testing) are these:Because we define the listener as a fat-arrow function
this
is not rebound to the current DOM element and remainsPhyloTree
(PhyloTreeType
).types don't match reality Changing the
NodeCallback
type to match reality,type NodeCallback = (this: PhyloTreeType, d: PhyloNode) => void
causes a very interesting type error (we're using@types/d3-selection
version 3.0.11):Overload 2,
(typenames: string, listener: (this: SVGPathElement, event: any, d: PhyloNode) => void, options?: any)
, is the signature of the current version of d3-selection, not the one we're using (note the first argument isevent
not the datum`). Overload 1 looks similarly wrong. I don't know why tsc doesn't show me overload 3.So it seems the type definitions we are using don't match the d3 libraries we're using.
The types seemingly forbid far-arrow functions
The type definitions (regardless of the fact that they are for a different d3 version than we're using) look like they enforce
this
being what d3 sets it to vialistener.call(this, ...);
which essentially forbids the use of fat-arrow functions (as they inherit thethis
in scope where the function was called from). This seems very limiting - fat-arrow functions are very useful for just this reason!