Declare package as ESM so import works on older Node - #160
Conversation
The `import` condition resolves to dist/idiomorph.esm.js, but the
package had no `type` field and the file has a .js extension. Node
calls that an ambiguous file and treats it as CommonJS unless module
syntax detection rescues it. Detection was unflagged in 22.7.0 and
backported to 20.19.0, so on node 18.x, 20.0-20.18 and 22.0-22.6
`import { Idiomorph } from "idiomorph"` fails to link with:
SyntaxError: Named export 'Idiomorph' not found.
825388e removed the `require` condition and `main`, which leaves the
mis-typed ESM file as the only entry point, so there is no longer a
CommonJS path to fall back to on those versions.
Declaring "type": "module" is what node recommends, since detection
costs an extra parse. It also disambiguates dist/idiomorph.js. The
field applies to every .js file in the package, so the two in-tree
CommonJS scripts move to .cjs. Renames only, contents untouched.
Closes bigskysoftware#158
75bfeb0 to
f493232
Compare
There was a problem hiding this comment.
Pull request overview
Declares the package as ESM to ensure import { Idiomorph } from "idiomorph" works reliably on Node versions that don’t have ESM syntax detection, and updates internal Node-run scripts accordingly. It also adds a CI smoke test that verifies the packed tarball imports correctly on Node 20.18.0 (and a modern Node).
Changes:
- Add
"type": "module"topackage.jsonso.jsentrypoints are unambiguously ESM for Node. - Rename the two in-repo CommonJS Node scripts to
.cjsand update npm scripts to call them. - Add a GitHub Actions job that packs and imports the tarball in a
type: moduleproject on Node 20.18.0 and 22.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
package.json |
Declares ESM package type and updates script paths to .cjs. |
test/lib/ensure-full-coverage.cjs |
CommonJS coverage-enforcement helper moved to .cjs. |
perf/runner.cjs |
CommonJS perf runner moved to .cjs. |
.github/workflows/ci.yml |
Adds an ESM entry smoke-test job using npm pack + import verification. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@myabc Thank you for explaining this, and for the PR. On one hand, those node versions are EOL, but on the other hand, its so simple to add Seems like the only drawback is needing to rename those two remaining CJS files to .cjs so that they still work. And really, I want idiomorph to only use ESM and <script src> so really I think the fix there would be to convert those files to ESM, thus finally closing the door on CJS. Is that reasonable to do here? I would prefer to not have the extra github action. So drop that and I'll merge, and if you feel like taking a crack at the CJS removal, go for it. Either here in this PR, or in a follow up, or I'll just merge the CJS rename and do it myself. |
|
@botandrose thanks for the feedback!
tbh I prefer that approach too. I just wanted to keep this PR tightly-scoped. I'll drop the GitHub Action. I think we can trust that ESM imports "just work" on all recent node versions once |
f493232 to
d5567bb
Compare
The package declares "type": "module", so the two node-run scripts had to be renamed to .cjs to keep working. Convert them to ESM instead and rename them back to .js, so no CommonJS is left in the repo. perf/runner.js drops __dirname in favour of resolving its two paths against import.meta.url directly. ensure-full-coverage.js takes lcov-parse's CommonJS export as a default import. Both are dev-only scripts; nothing here ships in the published tarball.
|
@myabc Looks great! Thank you! |
Fixes #158.
exports["."].importresolves todist/idiomorph.esm.js, but the package has no"type"field and the file has a.jsextension. Node calls that an ambiguous file and treats it as CommonJS unless module syntax detection rescues it. Detection was unflagged in 22.7.0 and backported to 20.19.0, so on Node 18.x, 20.0-20.18 and 22.0-22.6import { Idiomorph } from "idiomorph"fails to link withSyntaxError: Named export 'Idiomorph' not found.A repro with no flags and no bundler:
Same package, same file. The two Nodes disagree about its format because the package doesn't declare one. 825388e changed this: dropping the
requirecondition andmainleaves the ESM file as the only entry point, so those versions have no CommonJS path left to fall back to.The fix
"type": "module"inpackage.jsonis what Node recommends over relying on detection, which costs an extra parse. It also disambiguatesdist/idiomorph.jsanddist/idiomorph-ext.js. There are no dist renames, and no change tofiles,module,unpkg,typesorexports.The field applies to every
.jsfile in the package, so the two in-tree CommonJS scripts had to go. Rather than rename them to.cjs, they're converted to ESM:perf/runner.jsresolves its two paths againstimport.meta.urlinstead of__dirname, andtest/lib/ensure-full-coverage.jstakeslcov-parse's CommonJS export as a default import. Both are dev-only and ship in neither the published tarball nor the browser tests, which are served as<script>tags and never seepackage.json.<script src>and unpkg consumers are unaffected because browsers pick format by MIME type rather than bypackage.json.