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 packages are incompatible with Node projects that use "type": "module" #3469

Closed
Rich-Harris opened this issue Feb 16, 2021 · 28 comments · Fixed by #3506
Closed

D3 packages are incompatible with Node projects that use "type": "module" #3469

Rich-Harris opened this issue Feb 16, 2021 · 28 comments · Fixed by #3506

Comments

@Rich-Harris
Copy link

Rich-Harris commented Feb 16, 2021

I hope this is the right place to open this issue, as it's something that affects all d3-* packages as well as d3 itself. Please redirect me if not!

I've created a repo to provide a playground (where you can for example npm link a copy of d3-dsv with the proposed changes to see it working); this issue is a copy of that repo's README.


As of Node 12 (which is to say all maintained Node versions bar 10, which reaches EOL in April), it's possible for a Node project to use native JavaScript modules by adding the following to package.json:

{
  "type": "module"
}

Let's say you're making such a project, and you want to use (say) d3-dsv, and for extra points you're using TypeScript, so that your package.json looks like this...

{
  "devDependencies": {
    "@types/d3-dsv": "^2.0.1",
    "@types/node": "^14.14.28",
    "typescript": "^4.1.5"
  },
  "dependencies": {
    "d3-dsv": "^2.0.0"
  },
  "type": "module"
}

...and your tsconfig.json looks like this:

{
  "compilerOptions": {
    "allowJs": true,
    "moduleResolution": "node",
    "noEmit": true
  }
}

In a JavaScript file, which has // @ts-check at the top to enable typechecking, you import d3-dsv but discover that TypeScript yells at you:

Code with TypeScript error message

According to @types/d3-dsv, there's no default export — you need to use named imports instead. So we switch to using a * import, and now we get nice autocomplete...

TypeScript-powered autocomplete

...and typechecking:

TypeScript-powered typechecking

But when we run the program, it fails:

Error message

That's because Node believes that d3-dsv is a CommonJS package. When CommonJS is imported from ESM, Node treats module.exports as the default export. In other words, @types/d3-dsv disagrees with d3-dsv itself about what it exports.

It's not just TypeScript though — d3-dsv disagrees with itself about its exports. That's because (like all D3 packages) it follows the well-established convention of providing both pkg.main and pkg.module...

{
  "main": "dist/d3-dsv.js",
  "module": "src/index.js"
}

...which means that Node can ingest the CommonJS distributable version, while bundlers like Rollup ingest the modern source code. These bundlers, like TypeScript, expect to find named exports:

Rollup error message

In other words, if you're using native JavaScript modules, you have a choice: you can write D3 programs that run in Node, or you can write D3 programs that typecheck and can be bundled. You can't do both.

We can fix this!

Luckily this is easily solved, with one small caveat: we add "type": "module" to the package.json files in each package, alongside "exports" (which is how Node resolves e.g. d3-dsv to node_modules/d3-dsv/src/index.js):

  "main": "dist/d3-dsv.js",
  "unpkg": "dist/d3-dsv.min.js",
  "jsdelivr": "dist/d3-dsv.min.js",
  "module": "src/index.js",
+ "type": "module",
+ "exports": {
+   "./package.json": "./package.json",
+   "import": "./src/index.js",
+   "require": "./dist/d3-dsv.js"
+ },
  "bin": {

A new file, dist/package.json, allows the existing dist/*.js files to continue being treated as CommonJS (i.e. this cancels out the "type": "module":

{
  "type": "commonjs"
}

(In future, it might even be possible to get rid of the dist folder once native modules are universally adopted alongside import maps etc, but I'm not suggesting that any time soon given how D3 is used in the wild.)

The caveat is that this is technically a breaking change requiring a semver major release, since someone might currently be using these packages via import and would expect to be able to continue using the default import. We're talking about fairly extreme early adopters who can probably adapt very easily, and you could argue that the current behaviour is buggy and therefore can be fixed in a patch version. But if it was deemed necessary to support those users, it could be done in a non-breaking way by adding a default export alongside the named exports. For example:

-export {default as dsvFormat} from "./dsv.js";
-export {csvParse, csvParseRows, csvFormat, csvFormatBody, csvFormatRows, csvFormatRow, csvFormatValue} from "./csv.js";
-export {tsvParse, tsvParseRows, tsvFormat, tsvFormatBody, tsvFormatRows, tsvFormatRow, tsvFormatValue} from "./tsv.js";
-export {default as autoType} from "./autoType.js";
+import {default as dsvFormat} from "./dsv.js";
+import {csvParse, csvParseRows, csvFormat, csvFormatBody, csvFormatRows, csvFormatRow, csvFormatValue} from "./csv.js";
+import {tsvParse, tsvParseRows, tsvFormat, tsvFormatBody, tsvFormatRows, tsvFormatRow, tsvFormatValue} from "./tsv.js";
+import {default as autoType} from "./autoType.js";
+
+export {dsvFormat, csvParse, csvParseRows, csvFormat, csvFormatBody, csvFormatRows, csvFormatRow, csvFormatValue, tsvParse, tsvParseRows, tsvFormat, tsvFormatBody, tsvFormatRows, tsvFormatRow, tsvFormatValue, autoType};
+
+export default {dsvFormat, csvParse, csvParseRows, csvFormat, csvFormatBody, csvFormatRows, csvFormatRow, csvFormatValue, tsvParse, tsvParseRows, tsvFormat, tsvFormatBody, tsvFormatRows, tsvFormatRow, tsvFormatValue, autoType};

Thanks for reading this far! Obsessing over details like these is no-one's idea of a good time. However, now that the shift to a native module ecosystem is well and truly underway, more and more people will start to encounter these issues, and steps like these can minimise the frustration they experience.

D3 has shown impressive leadership in this area in the past and played a crucial role in helping to shift the JS ecosystem towards native modules, so this feels like a natural next step. Onward!

@doug3d

This comment has been minimized.

@Rich-Harris

This comment has been minimized.

@mbostock
Copy link
Member

Thanks for the detailed explanation, @Rich-Harris. I’m fully in support of doing this and indeed have been wanting to do this for some time, only being held back by the knowledge that this will almost certainly cause downstream breakages and complaints due to the wild and wonderful ways that JavaScript is consumed.

That said, all of the documentation clearly indicates that import * as d3 from "d3-foo" is the recommended pattern, so if someone is relying on an undocumented default export, I don’t think that’s worthy of triggering a major version bump.

@MylesBorins
Copy link

Quick nit.

The currently suggested package.exports won't work as expected.

You don't need to you "type": "module" to use ESM in node.js, you can simply use the .mjs file extension. What "type": "module" does is change the interpretation of the .js file extension to be ESM rather than CJS... this means that in a "type": "module" package you can't use the .js extension for CJS, you can instead use .cjs, which has the excellent slightly unexpected behavior of "just working" in old versions of Node.js as the runtime treats unknown file extensions as CJS (although you need to be explicit about the file extension in your source text, e.g. no automatic file extension resolution with .cjs). If you don't care about supporting Node.js < 12.20.x you can replicate some of the file extension magic using subpath patterns.

It might be more work than you are looking for but I have an example repo that has everything authored in ESM with the .mjs file extension and uses rollup to transpile a CJS version that is exposed using package.exports. This is a pattern you could explore using to simplify some of this, happy to answer any questions.

https://github.com/mylesborins/node-osc

One thing to keep in mind. If you choose to use package.exports folks will only be able to import files / paths that are defined as subpath exports. Folks mostly get bit by this when they discover they needed to explicitly export their package.json if folks are expecting to be able to import / require it.

Happy to answer any questions

@Rich-Harris
Copy link
Author

Rich-Harris commented Feb 17, 2021

@MylesBorins Ah whoops! Thanks for catching that, I should have written this instead to include package.json...

  "main": "dist/d3-dsv.js",
  "unpkg": "dist/d3-dsv.min.js",
  "jsdelivr": "dist/d3-dsv.min.js",
  "module": "src/index.js",
+ "type": "module",
+ "exports": {
+   "./package.json": "./package.json",
+   "import": "./src/index.js",
+   "require": "./dist/d3-dsv.cjs"
+ },
  "bin": {

...and specified that dist should contain a package.json that looks like this so that dist/d3-dsv.js et al are treated as CommonJS:

{
  "type": "commonjs"
}

I would have suggested .mjs, but there's a showstopper: VSCode doesn't know what to do with those files. Until the most popular editor gives you the same OOTB experience with .mjs as with .js, I don't think it's a realistic option for most people unfortunately.

Edited original comment to reflect this

@fregante
Copy link

fregante commented Feb 17, 2021

An alternative would be to drop the CJS module. What’s the advantage for a non-node module to keep CJS when it’s just generated from an ESM file and read only by bundlers anyway? To keep esmify-less browserify compatibility? Doesn’t seem worth the effort.

@marvinhagemeister
Copy link

Can confirm that what @MylesBorins suggestion is the easiest to implement and works in all environments. We tried a lot of different approaches over at Preact and found that using export fields with an mjs extension for the import source is the easiest to support without having to add "type": "module".

I would have suggested .mjs, but there's a showstopper: VSCode doesn't know what to do with those files. Until the most popular editor gives you the same OOTB experience with .mjs as with .js, I don't think it's a realistic option for most people unfortunately.

Agree, authoring in .mjs file is a poorer experience. We transpile/generate our dist/ files as usual and just rename the output files to add the .mjs extension via our build process.

@giltayar
Copy link

@marvinhagemeister just to put in my two cents: I found that using .js for ESM and .cjs for CJS is the best option (due to lack of support from VSCode and others), and is working wonderfully for me, including the occasional need for transpiling to dist/.

What were the problems you found with .js for ESM?

Also, while we're at it, my blog post on the whole ESM in Node.js experience: https://gils-blog.tayar.org/posts/using-jsm-esm-in-nodejs-a-practical-guide-part-1/

@Rich-Harris
Copy link
Author

@fregante because given the size of D3's userbase it's guaranteed that someone, somewhere, is doing this:

<script src="https://some.npm.cdn/d3"></script>
<script>
  d3.select(...)...
</script>

Removing the UMD dist files would break those apps — acceptable in a major version, but otherwise fraught.

@marvinhagemeister D3 packages don't expose a bundled ESM version, just the src. So the choice is

  1. rename all the src/**/*.js files to src/**/*.mjs (and live with the subpar authoring experience)
  2. start bundling ESM (and live with the increased complexity, and requirement to use sourcemaps etc)
  3. Use "type": "module" at the top level and "type": "commonjs:" inside dist

For me, 3 is an absolute no-brainer, but it's also not my decision.

@fregante
Copy link

fregante commented Feb 17, 2021

Removing the UMD dist files would break those apps — acceptable in a major version, but otherwise fraught.

Removing CJS isn’t necessarily the same as removing the file with globals. That file can live under the regular browser field. Also adding exports is a breaking change, so you might as well just use ESM with exports + globals in browser (or ask those CDN users to link to the browser file directly, unless they already do)

My understanding is that having both CJS and ESM means that you also have to have 2 definition types: one with export default for ESM and one with export =… unless you want to resort to esModuleInterop

@Rich-Harris
Copy link
Author

Ah, do you mean the d3.node.js file, aka pkg.main? (In d3-* packages, pkg.main is the UMD file.) You need to keep that, because otherwise this would fail:

const d3 = require('d3');

One day it'll be possible to remove support for require, but I don't think we're there yet.

@mourner
Copy link

mourner commented Feb 17, 2021

In my libraries, I decided to do what Sindre Sorhus is doing in his projects — slowly migrating them to ESM-only with "type": "module", no conditional exports, and dropping the CommonJS entry point and support for require completely (requiring Node v12+). UMD build is still generated for legacy uses (script tags etc) but it's not main.

@mourner
Copy link

mourner commented Feb 17, 2021

One day it'll be possible to remove support for require, but I don't think we're there yet.

@Rich-Harris I think now is the best time to do so. Node v10 reaches End of Life in a couple of months, and migrating libraries to ESM-only will get us to industry-wide adoption (and happy maintainer life) much faster. People still can do await import('d3') if they really need to use a ES module within a CommonJS one.

@Rich-Harris
Copy link
Author

@mourner I like the cut of your jib, and if that's the view of the maintainership then I enthusiastically support it. My only hesitation is that the people who can't yet adopt ESM in their own projects (for whatever reason) might blame D3 for the incompatibility. If it were to happen in a major version that's obviously less of a concern.

@mbostock
Copy link
Member

I like the idea of going all-in on ESM and doing it as a major version bump.

@curran
Copy link
Contributor

curran commented Feb 18, 2021

Would that jettison the UMD build, or keep the UMD build?

@mbostock
Copy link
Member

Keeping the UMD build in dist.

@mourner
Copy link

mourner commented Feb 19, 2021

My only hesitation is that the people who can't yet adopt ESM in their own projects (for whatever reason) might blame D3 for the incompatibility. If it were to happen in a major version that's obviously less of a concern.

@Rich-Harris yeah, definitely a valid concern, and such a change is certainly semver-major breaking. I would be worried about upgrading a package extensively used in the Node ecosystem (server-side utilities, infrastructure, etc.), but it seems like it's much less breaking for front-end libraries like d3, where require is usually used in the context of a bundler like Webpack, and is much easier to upgrade. Also, for popular projects, any major changes will cause some people to complain — it's inevitable, but when picking the time to eventually do the change, Node v10 EoL seems like the perfect moment to draw the line.

@curran
Copy link
Contributor

curran commented Feb 19, 2021

Definitely a concern. It reminds me of the current consistent stream of D3 "bug reports" people submit due to adoption of ES6.

@MylesBorins
Copy link

MylesBorins commented Feb 19, 2021 via email

@curran
Copy link
Contributor

curran commented Feb 19, 2021

Regarding my earlier comment on D3 "bug reports" people submit due to adoption of ES6 - I just want to clarify, perhaps it could have been way worse, and I'm not saying at all that it was a bad move to start adopting ES6. I just remember seeing so many little issues fly by like "It breaks in IE". Not sure what sort of baseline to expect, but the complaints on this front were definitely nonzero.

I just mean to say that, these are the same sort of issues that we can expect if we do a modernization change like this. If we could map out what changes would cause the most friction and make sure there are workarounds/solutions for each, we'd be golden. I bet most of the issues could be addressed by using all the modern approaches, but then saying "Hey, if it doesn't work for you, here's the UMD bundle that you can still use."

It would be interesting to map/test out the behaviors of various build tools with respect to the proposed changes, like:

  • Browserify
  • Webpack
  • Rollup
  • Popular project templates/systems like create-react-app, Next.js, Nuxt, Sapper.

n1ru4l added a commit to sikanhe/gqtx that referenced this issue Mar 1, 2021
* feat: introduce esm and cjs support according to d3/d3#3469 (comment)

* chore: fix example app

* chore: try to fix the typescript imports...
@wizzard0
Copy link

wizzard0 commented Mar 14, 2021

@mourner you need d3 cjs if your app is built with browserify (and switching bundlers for large projects is no mean feat) and also for serverside SVG rendering. So it's definitely semver-major.

@mbostock
Copy link
Member

I’ve started work on this with d3-format as a trial run: d3/d3-format#112 I’ll need to rewrite D3’s thousands of tests to use ES modules (switching from Tape to Mocha), which is the most tedious part.

@mourner
Copy link

mourner commented Apr 16, 2021

@mbostock I think it would be easier to keep using tape (than switch to Mocha) — the library itself works fine with ESM, it's just the binary runner that's not fixed yet, and there's both an existing workaround and a PR in the works.

@mbostock
Copy link
Member

Thanks for the tip, @mourner. I’ll look into that. I’ll need to fix the tests anyway (because they currently use require and would need to import), but that would certainly cut down on some of the churn.

@Fil
Copy link
Member

Fil commented Apr 28, 2021

@arthurblake-AngelOak
Copy link

arthurblake-AngelOak commented Jun 22, 2021

The main README for d3 still says:

In Node:

const d3 = require("d3");

You can also require individual modules and combine them into a d3 object using Object.assign:

const d3 = Object.assign({}, require("d3-format"), require("d3-geo"), require("d3-geo-projection"));

But both usages now seem to be broken.

@Fil
Copy link
Member

Fil commented Jun 25, 2021

// async method
(async function() {
  const d3 = Object.assign({}, await import("d3-format"), await import("d3-geo"), await import("d3-geo-projection"));
  console.log(d3);
})();
// needs "type": "module" in package.json
import * as d3Format from "d3-format";
import * as d3Geo from "d3-geo";
import * as d3GeoProjection from "d3-geo-projection";
const d3 = Object.assign({}, d3Format, d3Geo, d3GeoProjection);
console.log(d3);

Please review #3511

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

Successfully merging a pull request may close this issue.