Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

d3-selection dependency version issue with es6 #1823

Closed
nomego opened this issue Apr 2, 2021 · 8 comments · Fixed by #1824
Closed

d3-selection dependency version issue with es6 #1823

nomego opened this issue Apr 2, 2021 · 8 comments · Fixed by #1824

Comments

@nomego
Copy link
Contributor

nomego commented Apr 2, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

nomego commented Apr 3, 2021

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
Copy link
Contributor

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.

@gordonwoodhull gordonwoodhull reopened this Apr 3, 2021
@nomego
Copy link
Contributor Author

nomego commented Apr 3, 2021

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
Copy link
Contributor Author

nomego commented Apr 3, 2021

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

@kum-deepak
Copy link
Collaborator

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
Copy link
Collaborator

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:

  • Over the years I have realized that dc is used by very less (programming) skilled users to extremely skilled people. The library should work without fuss for less skilled people. For these users, ideally, the default UMD distribution should work with d3@5 as well as d3@6.
  • While using as es6 modules, I have a suspicion that it may not be possible to support both d3@6 and d3@5 transparently.
  • We can, for now, assume module users to have, at least, moderate programming skills.
  • Dynamic/conditional import (which is a function rather than a directive) might hinder the usage of standard UMD bundles, so, I think we should avoid it.
  • Probably, it will be good to keep the es6 modules to work out of the box with d3@6 with instructions (possibly include an alternate module for d3compat, or, to include an additional module) to use it with d3@5.

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

@nomego
Copy link
Contributor Author

nomego commented Apr 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants