-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix ESM exports #78
base: main
Are you sure you want to change the base?
Fix ESM exports #78
Conversation
any idea on when this will merge? |
Is there any updates on this PR? The workaround stated in #57 for {
// ...
mainFields: ["module", "main"]
} broke other parts, so the only way to resolve such error would be with this approach. |
This uses the `package.json` `exports` field to support native ESM. The `module` field is redundant and can even lead to compatibility issues, so it was removed. TypeScript project references are used for building the project with two different settings. An additional `package.json` is written to mark the ESM output as actual ESM. Since ESM requires file extensions, those were added to all imports. Mocha has been reconfigured to run tests for both CJS and ESM. Also ESLint has been configured to lint all files in the repo, not just a shell specific glob pattern. As a result, this change contains some small ESLint fixes.
3e127c2
to
ea96af8
Compare
I rebased and added the |
This is also blocking some downstream issues mentioned in microsoft/vscode#192144 |
Hi @aeschli, who needs to review this? |
I added an exports section with #90 |
That PR:
The latest version on npm is now broken.
|
"exports": { | ||
".": { | ||
"import": "./lib/esm/main.js", | ||
"module": "./lib/esm/main.js", |
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.
What's the meaning of "module"
exports?
I didn't see it in nodejs docs and webpack docs. Where is it used for or where is the official doc for it?
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.
It tells bundlers to use it, even when the module is require
d from a CJS file. So it helps reduce bundle size and avoid the dual package hazard.
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.
But the "import"
condition is more widely used in webpack, vite and node. Why should we add unofficial "module"
condition.
"compile": "tsc -p ./src && npm run lint", | ||
"compile-esm": "tsc -p ./src/tsconfig.esm.json", | ||
"prepack": "npm run clean && npm run test && npm run remove-sourcemap-refs", | ||
"compile": "tsc -b && node ./build/fix-esm.mjs && npm run lint", | ||
"remove-sourcemap-refs": "node ./build/remove-sourcemap-refs.js", |
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.
It seems that remove-sourcemap-refs job is useless. We can set "sourceMap": false
in tsconfig. @aeschli
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.
That breaks code coverage and source mapping when running tests.
anything still blocking this? |
@aeschli please note the issues referenced above |
The PR removes the AMD/UMD support. Until now I was hesitating to do that as the Monaco editor still uses AMD for its development setup.
|
Personally I would love to go ESM-only. I even advocate for it. But that would definitely be a breaking change. This affects more than just the VSCode ecosystem. I suggest to discuss the impact with the team internally. Perhaps @jakebailey also has an opinion on this. I’ll gladly update this PR accordingly |
There are other alternative packaging tools which would retain the other formats. I sort of suspect that someone's loading UMD in the browser or something, though I'm not sure how to determine that. If you go ESM only, just please do so in a major bump. Other breaking changes have happened in minor bumps and have been frustrating to work with, but dropping whole module formats would be extra breaky. |
This uses the
package.json
exports
field to support native ESM. Themodule
field is redundant and can even lead to compatibility issues, so it was removed.TypeScript project references are used for building the project with two different settings. An additional
package.json
is written to mark the ESM output as actual ESM. Since ESM requires file extensions, those were added to all imports.Mocha has been reconfigured to run tests for both CJS and ESM.
Also ESLint has been configured to lint all files in the repo, not just a shell specific glob pattern. As a result, this change contains some small ESLint fixes.
Closes #56
Closes #57