PMSI-AlignAlytics / dimple

An object-oriented API for business analytics
Other
2.73k stars 556 forks source link

draw throws exceptions for line charts #265

Open stephen-dunn opened 7 years ago

stephen-dunn commented 7 years ago

In the latest version dimple 2.3.0 and d3 4.3.0 draw() throws an exception when called after receiving a data update. here is an example

If I switch back to dimple 2.1.6 and d3 3.4.8 then it works as expected and does not throw an exception.

Here is the exception from the example above:

Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '.dimple-marker.dimple-series-0.' is not a valid selector.
tscizzle commented 7 years ago

I am getting this precise error as well.

Note that there doesn't even need to be a data update. Re-calling chart.draw at all yields this error.

To confirm, I did:

chart.draw();
console.log('drew once');
chart.draw();
console.log('drew twice');

and didn't get to the last print out, instead erroring as @stephen-dunn did.

peterj-freestar commented 7 years ago

I have been experiencing this error as well, and it prevents interactive filtering when rendering a dual-axis chart. When the user selects a filter value, from e.g. an element created with d3.select("#somedivid").append("div").append("select"), the first series gets redrawn but the DOMException interrupts the draw method and so the second series remains unchanged.

Confirming, though, that switching to d3.v3 and dimple.v2.1.6 solves the issue.

camille-s commented 7 years ago

I'm having this same problem with line charts as well, same error as given above. The best I can pinpoint, based on the error message '.dimple-marker.dimple-series-0.' is not a valid selector and checking in chrome dev tools, is that this comes from the method _drawMarkers. The section

if (series._markers === null || series._markers === undefined || series._markers[lineDataRow.keyString] === undefined) {
            markers = series._group.selectAll("." + markerClasses.join(".")).data(lineDataRow.markerData);
        } else {
            markers = series._markers[lineDataRow.keyString].data(lineDataRow.markerData, function (d) {
                return d.key;
            });
        }

looks like where names of classes are being joined by a dot. One of these dots is put at the end of the selector string--maybe some class in the array being joined is just blank text? Not sure the best way to fix it, but that seems to be where the issue lies.

guylando commented 6 years ago

tldr: THE FIX IS VERY SIMPLE: Change line 4411 from: series._markers[lineDataRow.keyString] = markers; To: series._markers[lineDataRow.keyString] = markers.merge(shapes); Change line 4311 from: series._markerBacks[lineDataRow.keyString] = markerBacks; To: series._markerBacks[lineDataRow.keyString] = markerBacks.merge(shapes); Change everywhere(Appears twice): series.shapes = series._group.selectAll("." + className); To: series.shapes = theseShapes.merge(entered);

It seems the problem is indeed in _drawMarkers. It is called twice once for "add", second time for "update" in the draw line function and in previous version the lines: // Deal with markers in the same way as main series to fix #28 if (series._markers === null || series._markers === undefined || series._markers[lineDataRow.keyString] === undefined) { markers = series._group.selectAll("." + markerClasses.join(".")).data(lineDataRow.markerData); } else { markers = series._markers[lineDataRow.keyString].data(lineDataRow.markerData, function (d) { return d.key; }); } // Add if (lineShape.nextSibling && lineShape.nextSibling.id) { shapes = markers.enter().insert("circle", '#' + lineShape.nextSibling.id); } else { shapes = markers.enter().append("circle"); } For update returned array of nulls which did not create elements. While in new version it DOES create new elements which don't have keyString and cause the exception later on because they keyString is undefined and is used to generate the class name.

The key differance with the new d3 version is: "The selection.sort and selection.data methods now return new selections rather than modifying the selection in-place." https://github.com/d3/d3/blob/master/CHANGES.md#selections-d3-selection And thus setting the data in the first conditions create new selection and thus the code afterwards thinks its new elements and d3 doesn't understand that its the old elements and it performs enter once again.

Calling markers.enter().append("circle") in new d3 will create new elements over and over again while in previous version it will not create anything (can put breakpoint in chrome devtools after the mentioned code block and test see this).

What we basically want to solve this is to save in the markers variable the markers in a way that calling d3 enter on existing markers will not cause them to re-enter.

SO FINALLY THE FIX IS VERY SIMPLE: Change line 4411 from: series._markers[lineDataRow.keyString] = markers; To: series._markers[lineDataRow.keyString] = markers.merge(shapes);

NOTE: SAME BUG IN _drawMarkerBacks: Change line 4311 from: series._markerBacks[lineDataRow.keyString] = markerBacks; To: series._markerBacks[lineDataRow.keyString] = markerBacks.merge(shapes);

NOTE: SEEMS THERE WAS A WRONG ATTEMPT TO HANDLE THIS BUG IN THE LINE SHAPES WHICH IN PREVIOUS VERSION WAS: series.shapes = theseShapes; And in current version it changed to: series.shapes = series._group.selectAll("." + className); WHICH IS WRONG AND CAUSES BUG BECAUSE LOSES INFORMATION SUCH AS KEYSTRING RESULTING IN THE CONSOLE ERROR ABOUT SELECT CLASS Need fix it to: series.shapes = theseShapes.merge(entered); (Appears twice)

huubbouma commented 6 years ago

@guylando I tried your fix and I can confirm that it works. Thanks!

aidandunsdon commented 4 months ago

Thank you a few years later!