-
Notifications
You must be signed in to change notification settings - Fork 22.9k
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
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 |
Quick nit. The currently suggested You don't need to you It might be more work than you are looking for but I have an example repo that has everything authored in ESM with the https://github.com/mylesborins/node-osc One thing to keep in mind. If you choose to use Happy to answer any questions |
@MylesBorins Ah whoops! Thanks for catching that, I should have written this instead to include "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 {
"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 |
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. |
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
Agree, authoring in .mjs file is a poorer experience. We transpile/generate our |
@marvinhagemeister just to put in my two cents: I found that using What were the problems you found with 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/ |
@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 @marvinhagemeister D3 packages don't expose a bundled ESM version, just the
For me, 3 is an absolute no-brainer, but it's also not my decision. |
Removing CJS isn’t necessarily the same as removing the file with globals. That file can live under the regular My understanding is that having both CJS and ESM means that you also have to have 2 definition types: one with |
Ah, do you mean the const d3 = require('d3'); One day it'll be possible to remove support for |
In my libraries, I decided to do what Sindre Sorhus is doing in his projects — slowly migrating them to ESM-only with |
@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 |
@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. |
I like the idea of going all-in on ESM and doing it as a major version bump. |
Would that jettison the UMD build, or keep the UMD build? |
Keeping the UMD build in dist. |
@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 |
Definitely a concern. It reminds me of the current consistent stream of D3 "bug reports" people submit due to adoption of ES6. |
I believe, but could be wrong, that dropping CJS support would potentially
make it significantly harder for those using browserify which still has
over 1 million downloads a month.
It seems like a UMD module that is included as `main` should allow
browserify to still work, but would be good to confirm that.
…On Fri, Feb 19, 2021 at 1:01 PM Curran Kelleher ***@***.***> wrote:
Definitely a concern. It reminds me of the current consistent stream of D3
"bug reports" people submit due to adoption of ES6.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3469 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV3TPBSHMJYZP7IBOWLS72RNZANCNFSM4XXJORCA>
.
|
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:
|
* feat: introduce esm and cjs support according to d3/d3#3469 (comment) * chore: fix example app * chore: try to fix the typescript imports...
@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. |
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. |
@mbostock I think it would be easier to keep using |
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. |
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. |
Please review #3511 |
- 7.0.0 has type:module issue (link: d3/d3#3469)
I hope this is the right place to open this issue, as it's something that affects all
d3-*
packages as well asd3
itself. Please redirect me if not!I've created a repo to provide a playground (where you can for example
npm link
a copy ofd3-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
: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 yourpackage.json
looks like this......and your
tsconfig.json
looks like this:In a JavaScript file, which has
// @ts-check
at the top to enable typechecking, you importd3-dsv
but discover that TypeScript yells at you: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......and typechecking:
But when we run the program, it fails:
That's because Node believes that
d3-dsv
is a CommonJS package. When CommonJS is imported from ESM, Node treatsmodule.exports
as the default export. In other words,@types/d3-dsv
disagrees withd3-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 bothpkg.main
andpkg.module
......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:
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 thepackage.json
files in each package, alongside"exports"
(which is how Node resolves e.g.d3-dsv
tonode_modules/d3-dsv/src/index.js
):A new file,
dist/package.json
, allows the existingdist/*.js
files to continue being treated as CommonJS (i.e. this cancels out the"type": "module"
:(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: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!
The text was updated successfully, but these errors were encountered: