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

fix(pnp): esm - return undefined source for commonjs #5677

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Aug 20, 2023

What's the problem this PR addresses?

The ESM loader always returns a source which after nodejs/node#47999 landed causes stuff to break.

How did you fix it?

Return undefined source for commonjs modules

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz added the esm label Aug 20, 2023
@arcanis
Copy link
Member

arcanis commented Aug 22, 2023

How can I repro the problem ?

@merceyz
Copy link
Member Author

merceyz commented Aug 22, 2023

Use Node.js nightly and run yarn in this repo, I've added a test so we can validate it regardless of what this repo uses in the future.

@merceyz merceyz force-pushed the merceyz/fix/pnp-esm-cjs branch from 8aafe09 to e19fd0c Compare August 22, 2023 18:00
@arcanis
Copy link
Member

arcanis commented Aug 22, 2023

That's what I did, but as of 62b2cf30f2d1326dde9d4bc047f5611f17c4a20f I couldn't repro 🤔

@merceyz
Copy link
Member Author

merceyz commented Aug 22, 2023

yarn was probably crashing due to other reasons when I tested it as it's fine now, it reproduces when running yarn test:integration pnp-esm.test.ts though.

I tested with nodejs/node@2557932 but can also reproduce with nodejs/node@62b2cf3.

@arcanis
Copy link
Member

arcanis commented Aug 23, 2023

Indeed; a minimal repro is:

node --loader ./loader.mjs <(echo '!!console.log(require.cache)')
import fs from 'fs';
import {fileURLToPath} from 'url';

export async function load(urlString, context, nextLoad) {
  return {
    format: `commonjs`,
    source: fs.readFileSync(fileURLToPath(urlString), `utf8`),
    shortCircuit: true
  };
}

@arcanis
Copy link
Member

arcanis commented Aug 23, 2023

@arcanis arcanis merged commit 08f6f70 into master Aug 23, 2023
@arcanis arcanis deleted the merceyz/fix/pnp-esm-cjs branch August 23, 2023 19:26
merceyz added a commit that referenced this pull request Aug 23, 2023
**What's the problem this PR addresses?**

The ESM loader always returns a source which after
nodejs/node#47999 landed causes stuff to break.

**How did you fix it?**

Return undefined source for commonjs modules

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
merceyz added a commit that referenced this pull request Aug 23, 2023
**What's the problem this PR addresses?**

The ESM loader always returns a source which after
nodejs/node#47999 landed causes stuff to break.

**How did you fix it?**

Return undefined source for commonjs modules

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants