Recently added `d3-strict` check balks on `parcoords` brush interaction with Dev Console open

[monfera]

Just checking this report by @alexcjohnson:

Uncaught Error: d3 selection.style called as getter: disallowed as it can fail for unattached elements. Use node.style.attribute instead.
    at Array.selProto.style (strict-d3.js:36)
    at SVGGElement.brushstart (d3.js:9209)
    at SVGGElement.__onmousedown.brush (d3.js:1120)

The test_dashboard seems to work for parcoords

> npm run start-test_dashboard

but indeed it shows the error in the console if Dev Tools are open.

It's good to avoid .style('something') or .attr('something') getters in favor of one-directional data flow. If we can read something this way, probably we put that info there to begin with, so it should be in the model / viewModel. I was curious how it got in there.

Turns out, D3 itself is doing this in the d3.svg.brush component which is used for the axis brush:

d3.select("body").style("cursor", eventTarget.style("cursor"));

This one seems to have a good motive, and also safe because the eventTarget must exist in the DOM in order to capture an event, and the event callback, being synchronous, can't be preceded by the target removal, and I'm wondering how we could exempt it.

To clarify, what's meant by unattached elements, simply, not being in the document tree? For reasons of possible leak avoidance and API compactness (I think) D3 doesn't separate createElement and attachment (appendChild or insertBefore) so an .append() will do both. So D3 isn't optimized for creation of entire DOM fragments for subsequent attachment or repeated attachment/removal (would be neat as it can reduce jank via less gc). Upon .remove() the node gets removed from the DOM but can't immediately get garbage collected because the D3 selection in v3 and v4 hangs onto it via a strong reference, so we can still query the detached node (v4):

https://codepen.io/anon/pen/ppVvVB

If the node got .removed and the corresponding selection in the lexical scope is gone (ie. GC happened and we might be in some subsequent function), then we can no longer reference it via that selection and have to perform a new selection, and with the node detached, the node won't be in it.

So it'd be good to know what case resulted in the creation of this rule; not knowing that, these might be some options, each has challenges so looking for input:

1. figure out a way for the rule to apply selectively for userland code vs D3 (seems hard, unwieldy or error prone)
2. find a way for D3 to not need this, make a PR for it and hope it gets merged (ifs)
3. reimplement brushing in our userland code (anti-DRY; bundle size; error prone)
4. catch the possible error in parcoords (but it's not needed in prod code)
5. disable the rule (it's a good rule though if it can catch legit problems)
6. not worry about it (would be sweeping it under the rug, only for completeness) as the strict test is run on initial renders and brushing is interactive, and in test_dashboard it's a problem only if Dev Tools is open and it's set to stop on errors

I tested some other plots with D3 based interactions (sankey, table) but they didn't have this issue. Any chart however might in theory have non-strict-d3 calls on interactions as it's a runtime check.

[monfera]

... just noticed that due to this validation, parcoords is degraded in test_dashboard even with closed Dev Tools because event processing becomes unfinished and the axis can be dragged sideways while brushing, unlike how the production version operates. Dragging an axis by its legend, and releasing it also makes the axis settle in its correct place, unlike in test_dashboard.

[alexcjohnson]

Thanks for the deep dive @monfera - the reason we made this rule was to ensure you could render a chart into an unattached <div>, and then attach that <div> to the DOM sometime later when it should become visible. This logic doesn't apply to interaction effects, since if you're interacting with a plot it must be visible!

We could drop strict-d3 from devtools, but keep it only for image tests. But there's something nice about having that check in devtools in order to catch problems earlier. Could we perhaps override d3.svg.brush to disable this prohibition while inside brush, and re-enable it when brush exits? Kind of hacky but maybe it would work, and would have no effect on prod code.

[monfera]

@alexcjohnson I took another look at this to see how your suggestion could work. As d3-strict simply monkey-patches around the d3.selection.prototype object, invoking the real thing if tests pass, it'd need to be aware of a test exception in runtime. I could perhaps somehow wrap around the d3.svg.brush setup call in parcoords but it's not when it should have an effect, as there's no violating call then. So I now only have bad ideas and worse ideas, let's rule them out one by one:

1. Inspect arguments.caller to see if actual D3 internals made the code vs. our userland code. Ruled out as arguments.caller no longer works esp. in strict mode, and it'd be a horrible solution even if it did.
2. Wrap around the invocation of d3.svg.brush in parcoords with something that would cause d3-strict to cache the exception so it can check against the cache in runtime (basically, a workaround for the above). I don't see how and what it could cache to make it work on the runtime event handling code dynamically. Also, it'd require that our userland (production) modules somehow are made aware of d3-strict which is probably not something we want.
3. Catch the currently thrown Error in d3-strict; inspect the stack trace in the error handling code with Error.prototype.stack, realize that it's a D3 internal code, let it go through. Problem: this relies on a generally supported, but non-standard property.
4. Make an exception for the 'cursor' property, hoping there won't be (a lot of) other exceptions as we add features. It's fairly unlikely that our code would read this property. It could still emit a warning.
5. Somehow make D3 use the original style method and our code the d3-strict version. But given the fact that D3 is object oriented and uses the fluent style, there's the gorilla-banana problem, almost everything returns a selection object so I don't see how we could go both ways on the same thing. And using modules in current JavaScript boils down to concatenated source strings in opaque functions for module boundaries, or not even that with Rollup.
6. Build the D3 bundle from sources shoehorned with a d3.svg.brush (and d3.selection) fork that would define, and call some .styleExempt method which we wouldn't override. But D3 v3 that we use isn't modular and it'd be a comparatively huge build burden and possible code divergence, doesn't feel like it's worth it. Also, this new method could leak into our userland code.
7. We could in theory make a PR toward the D3 repo, replacing this getter with an element.style.myStyle or node.getComputedStyle(node, null).getPropertyValue('myStyle') direct DOM API use, but it won't have a chance to be merged. Yet we could use this forked version. Probably a lower burden than item 6. But this makes me think that our developers can use direct DOM API calls any time, some of which might have the same limitation, so it's still not foolproof. Btw. I guess it's possible to
8. Statically check our source code against a pattern of .style(onlyOneThing), using regex (error prone) or the fantastic esprima (tedious, likely prohibitively time consuming to run on our large code base).

On the short term item 4 seems the least obtrusive to me but the above enumeration is shared to prime your creative thoughts, there may well be a better idea than these.