- Notifications
You must be signed in to change notification settings - Fork56
Fix ESM exports#78
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
graham-atom commentedOct 17, 2023
any idea on when this will merge? |
Yokozuna59 commentedNov 13, 2023
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 twodifferent settings. An additional `package.json` is written to mark theESM output as actual ESM. Since ESM requires file extensions, those wereadded 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 justa shell specific glob pattern. As a result, this change contains somesmall ESLint fixes.
I rebased and added the |
This is also blocking some downstream issues mentioned inmicrosoft/vscode#192144 |
afonsojramos commentedMay 22, 2024
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", |
xiaoxiangmoeJun 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 isrequired 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.
adrian-gierakowski commentedAug 12, 2024
anything still blocking this? |
afonsojramos commentedAug 13, 2024
@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. |
With the deprecation of Node.js 18, all supported Node.js versions support Dropping backwards compatibility with Node.js <20 is a breaking change of course. This would require a semver major release. |
Uh oh!
There was an error while loading.Please reload this page.
This uses the
package.jsonexportsfield to support native ESM. Themodulefield 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.jsonis 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