airbnb / visx

🐯 visx | visualization components
https://airbnb.io/visx
MIT License
19.53k stars 719 forks source link

d3 v7 #1267

Open kachkaev opened 3 years ago

kachkaev commented 3 years ago

Hi folks πŸ‘‹

There is a new major version of d3, which was released in June 2021: https://github.com/d3/d3/releases/tag/v7.0.0

A number of auxiliary d3 libraries got upgraded too, e.g. d3-array: v2 β†’ v3 etc.

I work on a project that uses a few d3 libraries directly and also via visx. Thus, upgrading direct dependencies to the latest versions produces avoidable duplicates.

What is visx’s strategy on d3 dependencies? When would be a good time for you to upgrade them? Happy to help with a PR if it is needed and if time allows πŸ™‚

williaster commented 3 years ago

Hey @kachkaev thanks for opening this πŸ™

We're definitely open to updating the d3 deps. From a quick glance it doesn't look like there are too many crazy breaking changes. We don't have the whole d3 package installed anywhere, so I think it'd probably make sense to update d3-* dependencies on a per-visx package basis. I think for TS to be happy we'd hope to keep the d3 types in sync as well, it looks like there was a 7.0.0 release for those as well.

I'm not sure I have the bandwidth to do this currently but am happy to review PR(s) if others are interested πŸ‘€

azemetre commented 3 years ago

If no one else is taking this, I don't mind working on it.

I'll tag this issue in an open PR once I have it complete.

edit:

after updating the packages there were some issues with the current node engine not supporting the newer d3 libs using ESM, bumping it the node version to 12.22.4 resolved this issue.

But I ran into some problems with running visx-demo. next.js does not seem to like any of the D3 libraries (shape, scale, format, etc) using ESM:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/azemetre/github/visx/node_modules/d3-shape/src/index.js
require() of ES modules is not supported.
require() of /Users/azemetre/github/visx/node_modules/d3-shape/src/index.js from /Users/azemetre/github/visx/packages/visx-shape/lib/util/D3ShapeFactories.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/azemetre/github/visx/node_modules/d3-shape/package.json.

Kinda don't know the best approach to solve this, any suggestions? This is the last hurdle. It's able to build and tests pass with newer D3 libs but don't know how to handle next.js ESM woes.

edit2: tried to jump ahead and update react to v17 and was also trying out nextjs v11.0.2-canary.27 (supports esm).

Only issues I had with using the canary build of nextjs was improper react installations (had multiple local copies, had issues properly building; totally on my end, don't have the time to properly resolve). Might be something to consider if it requires minimal breaking changes (had to tweak the next.config.js to allow webpack v4).

Might be better to revisit this issue after these two PRs are merged in tho:

https://github.com/airbnb/visx/pull/1268

https://github.com/airbnb/visx/pull/1301

make-github-pseudonymous-again commented 3 years ago

Kinda don't know the best approach to solve this, any suggestions? This is the last hurdle. It's able to build and tests pass with newer D3 libs but don't know how to handle next.js ESM woes.

I believe the alternatives are:

  1. Have d3 provide a .cjs file either in package.json#main or package.json#exports so that it can be picked up by node when using require.
  2. Use await import(...) (not top-level) instead of require where it breaks, this may not be compatible with the existing synchronous API, or be easy to produce from TypeScript sources. Not sure what's the support for this in NodeJS 12.x either.
  3. Only provide ESM output for @visx libraries.

IMHO, the only reasonable alternative is 1.

azemetre commented 3 years ago

Kinda don't know the best approach to solve this, any suggestions? This is the last hurdle. It's able to build and tests pass with newer D3 libs but don't know how to handle next.js ESM woes.

I believe the alternatives are:

  1. Have d3 provide a .cjs file either in package.json#main or package.json#exports so that it can be picked up by node when using require.
  2. Use await import(...) (not top-level) instead of require where it breaks, this may not be compatible with the existing synchronous API, or be easy to produce from TypeScript sources. Not sure what's the support for this in NodeJS 12.x either.
  3. Only provide ESM output for @visx libraries.

IMHO, the only reasonable alternative is 1.

Awesome, I can try out again this weekend and report back.

becca-bailey commented 1 year ago

Hey! πŸ‘‹ I have seen this before and I thought I would share some things we learned from upgrading Victory (Formidable's React + SVG charting library) to the latest version of d3.

As you have figured out here, the latest version of d3 no longer supports CommonJS, which is required for node.js environments. Since d3 itself is unlikely to change this (we also went that route), we created a script to vendor our d3 dependencies to export both CommonJS and ESM, and updated our imports to use our vendored d3 packages rather than importing directly from node_modules/d3. My colleague at the time wrote a really helpful blog post that goes into more detail about this approach. https://formidable.com/blog/2022/victory-esm/

@williaster If we wanted to take a similar approach here, I'm happy to take a stab at it! Let me know what you think.

williaster commented 1 year ago

hey @becca-bailey thanks for chiming in – I've read that post and it was great πŸ™ in our 3.0.0 release we updated a few d3 deps to the latest to fix some security vulnerabilities, and now we're in a semi-broken state with regard to supporting CSJ 😞

we've discussed 3 options in https://github.com/airbnb/visx/issues/1637, the first I wanted to try was updating all of our problematic d3-* imports to import() to work in both csj/esm worlds – but it's unclear if that will work for all consumers. we haven't discussed making our own vendor packages which could be interesting. @becca-bailey how much work do you think it'd be to make @visx/vendor with cjs/esm exports which we can then use for all of our other imports? happy to review or collab on a PR to explore. (feel free to chime in on the other issue thread as well)

kaiyoma commented 6 months ago

Will this ever be addressed? I'm trying to upgrade to D3 7.x in my project and running into TypeScript errors because visx pulls in an ancient version of d3-shape (1.x, 5 years old).

williaster commented 6 months ago

@kaiyoma we now support d3's esm-only packages via @visx/vendor. so updating to the latest d3 packages should be doable. if you or anyone is interested in an updating a given package you can follow the instructions here https://github.com/airbnb/visx/issues/1761#issuecomment-1784225329