-
Notifications
You must be signed in to change notification settings - Fork 43
feat: output uses "type": "module" when esModule is set to true
#478
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
base: main
Are you sure you want to change the base?
Conversation
This change enables ES module compatibility by automatically adding "type": "module" to the generated package.json when ESM output is included. The test runner code has also been updated to handle ES module environments by adding the necessary import statements and compatibility shims. Key changes: - Modified package.json generation to include "type": "module" field when includeEsModule is true - Updated test runner code generation to add ES module compatibility imports (createRequire, __dirname polyfill) - Updated all test expectations to reflect the new "type": "module" field in generated package.json files This ensures that generated npm packages work correctly in ES module environments while maintaining backward compatibility.
| if (options.includeEsModule) { | ||
| // ensure compatibility with esm ("type": "module") | ||
| writer.writeLine(`import { createRequire } from 'module';`); | ||
| writer.writeLine(`const require = createRequire(import.meta.url);`); | ||
| writer.writeLine( | ||
| `const __dirname = new URL(".", import.meta.url).pathname;`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered polyfilling the safer way to enable ESM compatibility instead of outputting ES modules directly.
| const type = { | ||
| ...(includeEsModule | ||
| ? { | ||
| type: "module", | ||
| } | ||
| : {}), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includes "type": "module" if esModule: true, otherwise doesn't add the type field at all.
It also still allows the user to explictly overwrite the type field via the build function's config.
| // Copyright 2018-2024 the Deno authors. MIT license. | ||
|
|
||
| import { parse } from "jsr:@std/csv/parse"; | ||
| import { parse } from "jsr:@std/csv@1.0.6/parse"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a version specifier to fix a linter error. The version used is based on the latest @std/csv version specified in deno.lock.
| "no-explicit-any", | ||
| "camelcase" | ||
| "camelcase", | ||
| "no-import-prefix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run into linter issues with the existing code using inline imports. This rule got added to the default ruleset in Deno 2.5.
My assumption is the project's okay with inline imports, so I decided to exclude the rule instead. Let me know if I should rather refactor inline imports instead.
|
Tests are failing on Windows. Let me take a look later 👍 |
Summary:
Sets output package.json
typeto bemodulewhenesModule: trueis used. This ensures the output package is automatically ESM-compatible.Also fixes ESM compat for the generated
test_runner.jsfile by polyfillingrequireand__dirnamein an ESM context.Details:
Users targeting ESM output now have to explicitly add
"type": "module"to the build function's configuration. This makes the output ESM compatible, but breaks the generated test runner.This is potentially a breaking change because of
"type": "module"implications in the output package.Fixes #476
Checks:
deno task test) passing with Node versions 18-24