dc-js / dc.js

Multi-Dimensional charting built to work natively with crossfilter rendered with d3.js
Apache License 2.0
7.41k stars 1.81k forks source link

d3-selection dependency version issue with es6 #1823

Closed nomego closed 3 years ago

nomego commented 3 years ago

In an effort to try and fix #1822 locally, I did an npm install --save d3-collection which installed v1.0.7.

This stopped the server from erroring out but instead I get this in the console:

Uncaught SyntaxError: The requested module './../../../d3-selection/src/index.js' does not provide an export named 'event' from d3compat.js:1.

Looking at the installed version of d3-selection it's v2.0.0 where the event export seems to be gone. Downgrading with npm install --save d3-selection@1.4.1, event is available but it fails with Uncaught SyntaxError: The requested module './../../../d3-selection/src/index.js' does not provide an export named 'pointer' on the same line, which seems to be an addition of v2.0.0.

Not sure how this dep should be rectified.

gordonwoodhull commented 3 years ago

Like I said in #1822, d3-collection is obsolete and incompatible with the latest D3. The correct solution does not involve installing d3-collection.

The only workarounds that I can can think of from the top of my head involve modifying the file in question. (Specifically, deleting the offending import.)

nomego commented 3 years ago

Sorry if the issue description was confusing but this actually doesn't have anything to do with d3-collection, maybe I shouldn't have mentioned the other issue. This is regarding d3-selection. If dc.js is meant to support es module usage, when importing for example BarChart, it has a dependency to d3compat, which in turn needs to:

import {event, mouse, pointer} from 'd3-selection';

But I can't find a d3-selection version that exports those three. The automatically installed v2.0.0 doesn't have event and downgrading to v1.4.1 loses pointer instead.

gordonwoodhull commented 3 years ago

I see. It uses the same technique as the other issue, expecting to find undefined when the symbol is not defined. And it occurs in the same file, which is used for backward compatibility with d3@5.

But it refers to a different module, one which still exists but has different exports now.

I guess rollup-generated code is more lenient then native ES6 in this regard.

nomego commented 3 years ago

Yes I'm pretty sure the spec behavior is to fail hard, but it sounds like rollup can be configured to gracefully fail.

How about designing the code for a d3@6 environment and then including a compat-layer completely separate, only used in d3@5 (and lower?) contexts? It would then be included in the rollup build to produce the equivalent of the current output, if all consumption of this library is through a built rollup version anyway? Or there could even easily be two separate v5- and v6+ compiles. It would highlight the dependency structure a lot more clearly, the contract is very hidden right now.

I would propose to evaluate something like registering a dcjs namespace in globalThis and optionally registering things like adaptHandlers or nestHandlers there to override behavior. In the library you'd then check for those handlers and use if specified, otherwise default to latest (v6) behavior. This way users can use the rollup build as they do now but for es module approaches like mine I would not need any compat layer, if I did, I'd import the compat layer before the main dc.js library. That would again cause conflicting v5+v6 imports and to solve that the v6 behavior would have to be its own compat layer, forcing the user to always have a compat layer import, which might not be a bad thing, it's definitely better than not working at all as it is now.

What do you think?

nomego commented 3 years ago

Ah sorry, realized you have dc.config, which would make more sense than globalThis.

kum-deepak commented 3 years ago

I see. It uses the same technique as the other issue, expecting to find undefined when the symbol is not defined. And it occurs in the same file, which is used for backward compatibility with d3@5.

But it refers to a different module, one which still exists but has different exports now.

I guess rollup-generated code is more lenient then native ES6 in this regard.

The rollup, currently, rewrites imports for any of the d3-* modules to d3. In addition, the UMD is typically used in browser context via script tags (both d3 and dc), so, it works.

kum-deepak commented 3 years ago

Yes I'm pretty sure the spec behavior is to fail hard, but it sounds like rollup can be configured to gracefully fail.

How about designing the code for a d3@6 environment and then including a compat-layer completely separate, only used in d3@5 (and lower?) contexts? It would then be included in the rollup build to produce the equivalent of the current output, if all consumption of this library is through a built rollup version anyway? Or there could even easily be two separate v5- and v6+ compiles. It would highlight the dependency structure a lot more clearly, the contract is very hidden right now.

I would propose to evaluate something like registering a dcjs namespace in globalThis and optionally registering things like adaptHandlers or nestHandlers there to override behavior. In the library you'd then check for those handlers and use if specified, otherwise default to latest (v6) behavior. This way users can use the rollup build as they do now but for es module approaches like mine I would not need any compat layer, if I did, I'd import the compat layer before the main dc.js library. That would again cause conflicting v5+v6 imports and to solve that the v6 behavior would have to be its own compat layer, forcing the user to always have a compat layer import, which might not be a bad thing, it's definitely better than not working at all as it is now.

What do you think?

Thanks for analyzing possible approaches. I would like to add the following constraints:

@nomego, you have already analyzed it quite beautifully. Would you work on a PR where I can contribute as needed?

nomego commented 3 years ago

Yes of course it should work dynamically like today, where the dist version adapts dynamically to d3@5/d3@6. You've made an interesting built system where I assume it builds with @6 and fails for all things @5, but does so gracefully and converts d3 imports to d3 global references, causing it to "just work" in an @5 settings; clever. :)

I made a PR for an example approach which needs some discussion and review in #1824, let me know what you think.