feat: support ESM - Githubissues

nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers

MIT License

2.49k stars 162 forks source link

feat: support ESM #270

Closed merceyz closed 1 year ago

merceyz commented 1 year ago

What's the problem this PR addresses?

Corepack doesn't support package managers using ESM since it always loads them as CJS modules. This isn't a problem at the moment but might be in the future.
https://github.com/nodejs/corepack/pull/268 made Corepack preserve process.exitCode when the package manager throws synchronously which Node.js doesn't do by default.
Corepack sends synchronously thrown errors by package managers to stdout instead of stderr.

How did you fix it?

Start package managers using Module.runMain.

arcanis commented 1 year ago

Sounds good, but I think I'll merge it after the release, as it seems a little more dangerous than the two other changes

merceyz commented 1 year ago

All the tests pass so this PR should be just as "safe" as what's currently in main. If anything it should be safer as now we're using the same code path as Node.js instead of our own incorrect one.

arcanis commented 1 year ago

I meant "safe" more in terms of "likelihood that it could have an unplanned effect" - both other changes seem low likelihood / high value (making them something that we want released in Node asap), whereas this one has more significant changes. In the unlikely-but-more-likely-than-regular-fixes event we'd have to revert this PR, I'd prefer if we still had the two fixes.

If anything it should be safer as now we're using the same code path as Node.js instead of our own incorrect one.

True, but the current code path is "known" and worked so far, so it's not high-priority to ship it in the same release (note that I'm not against making two releases the same day - I'd just prefer to ship them as two different PRs to the Node repository, in case a rollback is needed).