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): always initialize the ESM loader when enabled #3667

Merged
merged 9 commits into from
Nov 10, 2021

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Oct 28, 2021

What's the problem this PR addresses?

Dynamic imports doesn't work in commonjs files when the entrypoint is also a commonjs module.

Fixes #3671
Fixes #3687
Fixes PnP support in the Babel repo (cc @nicolo-ribaudo)
Fixes (part of) the failing Angular e2e test

How did you fix it?

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.

);

test(
`it should always set default as module.exports when importing a commonjs file`,
Copy link
Member

@arcanis arcanis Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note: it's worth adding a (in-PR) comment when tests are removed, explaining why they aren't relevant anymore (were they bugged?) 😃

Copy link
Member Author

@merceyz merceyz Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I documented it in the commit that removed them

@timmywil
Copy link

Could this be released to canary?

@merceyz
Copy link
Member Author

merceyz commented Nov 16, 2021

Sure, running it now https://github.com/yarnpkg/berry/actions/runs/1468300994

Alternatively you can get it from sources

yarn set version from sources

@teohhanhui
Copy link

@merceyz
Copy link
Member Author

merceyz commented Nov 17, 2021

The publish to the registry failed but set version... doesn't use the registry so it doesn't matter.

yarn set version canary

@kherock
Copy link
Contributor

kherock commented Nov 18, 2021

The extensionless file workaround for nodejs/node#33226 in this PR doesn't work when I'm attempting to load an entrypoint with an arbitrary file extension that's intended to be parsed by a require hook like babel-register. I have a local package that loads an entrypoint with a .ts extension that should normally load as CJS.

Are there any gotchas to renaming the ``: and `.js`: cases to the default: case? I expect .ts to load as CJS when the package doesn't declare "type": "module". This assumption might change once .mts exists, but for now defaulting to the package type feels like the most correct behavior.

@merceyz
Copy link
Member Author

merceyz commented Nov 21, 2021

Could you open a seperate issue to track that?

Are there any gotchas to renaming the ``: and .js: cases to the default: case?

We don't want to deviate from how Node behaves more than we have to so we could only do that for the entrypoint

@tavoyne
Copy link
Contributor

tavoyne commented Jan 29, 2022

I'm experiencing an analogous issue that this PR doesn't seem to be fixing.

I'm trying to use the @mdx-js/loader@2.0.0-rc.2 webpack loader. In the package's entry point (it's a .cjs file), there's a dynamic import of an ES module that causes the following error:

node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/blackjelly/code/blog-3/.yarn/__virtual__/@mdx-js-loader-virtual-a537165eb1/0/cache/@mdx-js-loader-npm-2.0.0-rc.2-eb4487e27d-5637b8d92c.zip/node_modules/@mdx-js/loader/lib/index.js' imported from /Users/blackjelly/code/blog-3/.yarn/__virtual__/@mdx-js-loader-virtual-a537165eb1/0/cache/@mdx-js-loader-npm-2.0.0-rc.2-eb4487e27d-5637b8d92c.zip/node_modules/@mdx-js/loader/index.cjs

Which setting Yarn's version from sources or using canary doesn't fix.

@merceyz
Copy link
Member Author

merceyz commented Jan 29, 2022

Please open a bug report with a reproduction

@yarnpkg yarnpkg locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: PnP fails to resolve dynamic require from CJS [Bug?]: cannot import chai from a module using PNP
6 participants