Cure ES Module Madness #203
Conversation
A number of JavaScript files were rewritten as ES Modules, but their extensions remained `.js`. That extension is ambiguous to newer version of the Node.js runtime, which can load both CommonJS modules and ES Modules. We could disambiguate by renaming CommonJS modules to `.cjs` files and ES Modules to `.mjs` files, per Node.js convention. Alternatively, we can add `package.json` files with `type` properties to the various subdirectories. Setting `type` to `"module"` tells Node.js to interpret `.js` files in that directory and below as ES Modules. Otherwise, Node.js falls back on the `package.json` at root, which currently sets `type` to `"commonjs"`. I've done the latter here, to avoid renames in Git history.
This reverts commit 9bcf45b. Node.js version 14, which supports ES Modules without any flag or the `esm` package, is currently in long-term support. But a great many folks still run older version of Node.js that either don't support ES Modules at all or hide that support behind a feature flag. We could instruct Node.js to load the bin script as an ES Module via a flag in shebang, but that flag won't be available on older versions of Node.js. Meanwhile, this bin script doesn't stand to gain anything from moving to ES Modules. When something runs the bin script, it's going to be Node.js, not a browser. So we might as well use `require` and stop worrying about ES Modules.
`lib/index.js` is now specifically the loading entry point for
loading as an ES Module via `import`. Using `require('../')` instead
will ensure that Node.js follows the loading configuration in
`package.json`.
This might help bundlers reduce bundle size by specifying specific imports. I haven't tested it empirically, but I understand this is best practice for loading with `import` anyway.
|
Ref: #201 |
|
@jgm, by the by: Greetings from North Oakland. Hope this module madness hasn't gotten to you too bad. It really is a mess, and none of it's the fault of this package. |
| if (os.platform() === "win32") { | ||
| inps.push(fs.readFileSync(0, 'utf-8')); | ||
| } else { | ||
| inps.push(fs.readFileSync('/dev/tty', 'utf-8')); | ||
| } | ||
| } else { | ||
| for (i = 0; i < files.length; i++) { | ||
| var file = files[i]; | ||
| inps.push(fs.readFileSync(file, 'utf8')); | ||
| } | ||
| files = ['/dev/stdin']; | ||
| } | ||
|
|
||
| for (i = 0; i < files.length; i++) { | ||
| var file = files[i]; | ||
| inps.push(fs.readFileSync(file, 'utf8')); |
There was a problem hiding this comment.
Was there a reason for removing the platform-specific handling here?
cf. discussion in #198.
There was a problem hiding this comment.
No reason for removing the platform stuff in the bin script. Looks like it was done in the same commit as ESM-ification: 9bcf45b. So git revert rolled those changes back, too.
I can replace them, or you can fee free to. Whatever's easiest for you.
There was a problem hiding this comment.
I'm not really sure why this got included with the other changes.
Well, I guess I can add them back after mrging.
|
Thanks a lot for this! |
|
@kemitchell do you see what is causing the CI failures here? |
|
@jgm, CI on Node 10 and 11 is failing because the tests were ported to ES Modules, and those versions of Node can't parse them. Something else is going on on Node 13. |
|
PR coming. |
This PR attempts to resolve various issues and errors affecting bin script execution and loading via
importin Node.js. It doesn't touch the existing conditional-import ormodulebunlder entry point configurations inpackage.jsonat root.Overall, the blunt instrument at work here is
package.jsonfiles with"type":"module"set. In order to make clear to Node.js what is an old CommonJS module and what is a new ES Module, we have just a few choices:.cjsfor CommonJS and.mjsfor ES Modules. But old versions of Node don't know about this convention.typeto"commonjs"or"module"inpackage.json, at package root, in subdirectories, or both. Newer versions of Node will look at the nearestpackage.jsonand load.jsfiles according to thetypespecifier. Cluttering, but effective.For maximum compatibility and minimum renaming things, I've opted to add
package.jsonfiles in several sudirectories. Thepackage.jsonat root continues to have"type":"commonjs". But that is overridden forlib,test, andbench, which have been rewritten as ES Modules. I did not add suchpackage.jsonfiles todistordingus.dist/commonmark.jsis the entry point when folks userequireto load this package, so it is and should be treated as CommonJS.As for the bin script, which will always be run by Node.js, and not a browser, I don't see any practical advantages to ES Modules right now. If and when this package drops support for Node.js <= 13, the bin script can be safely rewritten with
import. But for now,requireworks fine and doesn't cause compat problems.Preloading the
esmpackage via a Node.js flag might work in many cases, but it's not necessary. Downloading and loadingesmis a kludge for those on old versions of Node.js, and wholly unnecessary for those on 14, 15, or later. As a general matter, I like to keep packages out of the decision making process about whether and how to configure module loading for any module but the one the package offers. That's something that users of the package can decide on and configure for themselves and all their dependencies.I've included more detailed discussions in commit messages.