Skip to content
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

feat: support ESM #270

Merged
merged 3 commits into from
Jun 13, 2023
Merged

feat: support ESM #270

merged 3 commits into from
Jun 13, 2023

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented May 26, 2023

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.
  • fix: don't override process.exitCode #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
Copy link
Contributor

arcanis commented Jun 13, 2023

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
Copy link
Member Author

merceyz commented Jun 13, 2023

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
Copy link
Contributor

arcanis commented Jun 13, 2023

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).

@arcanis arcanis merged commit be2489c into nodejs:main Jun 13, 2023
@merceyz merceyz deleted the merceyz/feat/esm-support branch June 13, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants