[Bug?]: Dynamic import does not use the PnP API when importing a ESM from CommonJS

[flying-sheep]

Self-service

• I'd be willing to implement a fix

Describe the bug

Trying to use the https://github.com/xojs/vscode-linter-xo extension with yarn 2, I ran into problems in the past, see #3708. With nodejs 17.4, one blocker is gone, but it doesn’t work yet:

The yarn PNP loader doesn’t seem to be used when a dynamic import from a commonjs module imports a ESM like in this dynamic import:

const { Files } = require('vscode-languageserver/node')
const { URI } = require('vscode-uri')

async function main() {
	const xoFilePath = await Files.resolve('xo', undefined, '.')
	const xoUri = URI.file(xoFilePath).toString()
	await import(xoUri)
}

main()

To reproduce

Sadly I don’t think reproducing using sherlock is possible, as this requires running node with NODE_OPTIONS set, and sherlock doesn’t seem to support that. I made a reproducer in https://github.com/flying-sheep/repro-yarn-xo

1. git clone https://github.com/flying-sheep/repro-yarn-xo.git && cd repro-yarn-xo

2. yarn start (i.e. bash .yarn/sdks/xo/node.sh import-xo.js)

3. See the following error:

   node:fs:1538
     handleErrorFromBinding(ctx);
     ^
   
   Error: ENOTDIR: not a directory, stat '/home/phil/Dev/Web/repro-yarn-xo/.yarn/cache/xo-npm-0.47.0-3e655742d0-5bd4d72322.zip/node_modules/xo/index.js'
       at statSync (node:fs:1538:3)
       at tryStatSync (node:internal/modules/esm/resolve:160:13)
       at finalizeResolution (node:internal/modules/esm/resolve:387:17)
       at moduleResolve (node:internal/modules/esm/resolve:944:10)
       at defaultResolve (node:internal/modules/esm/resolve:1041:11)
       at ESMLoader.resolve (node:internal/modules/esm/loader:530:30)
       at ESMLoader.getModuleJob (node:internal/modules/esm/loader:251:18)
       at ESMLoader.import (node:internal/modules/esm/loader:332:22)
       at importModuleDynamically (node:internal/modules/cjs/loader:1036:29)
       at importModuleDynamicallyWrapper (node:internal/vm/module:437:21) {
     errno: -20,
     syscall: 'stat',
     code: 'ENOTDIR',
     path: '/home/phil/Dev/Web/repro-yarn-xo/.yarn/cache/xo-npm-0.47.0-3e655742d0-5bd4d72322.zip/node_modules/xo/index.js'
   }
   
   Node.js v17.4.0
   

Environment

System:
    OS: Linux 5.16 Arch Linux
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
  Binaries:
    Node: 17.4.0 - /tmp/xfs-b43fe39f/node
    Yarn: 3.1.1 / 3.2.0-rc2 - /tmp/xfs-b43fe39f/yarn
    npm: 8.4.0 - /usr/bin/npm

Additional context

No response

[merceyz]

It's because the PnP linker explicitly lists .yarn/sdks/** as a path it shouldn't control

berry/packages/plugin-pnp/sources/PnpLinker.ts

Line 280 in a248d0b

const ignorePattern = miscUtils.buildIgnorePattern([`.yarn/sdks/**`, ...this.opts.project.configuration.get(`pnpIgnorePatterns`)]);

The solution would be to remove it but maybe @arcanis remembers why its there

[flying-sheep]

That’s not true, the script within .yarn/sdks is a bash script, there’s no JS code in there. I just tried it out by moving the bash script out of .yarn/sdks: It doesn’t prevent the error.

I think the problem really has to do with node’s import behavior, as the error only happens when trying to dynamically import xo (a ESM) from commonjs. Using await import(xoUri) from another ESM works fine.

Since the SDK folder thing is demonstrably unrelated to this, I took the liberty to change the title back.

[arcanis]

The solution would be to remove it but maybe @arcanis remembers why its there

Was added in #646; I don't find a reference to NODE_PATH anymore, so perhaps this isn't needed anymore?

[merceyz]

Ah, I was looking at the repro from #3708 sorry about that.

This was fixed in #3667 so this is a duplicate of #3687

yarn set version canary && yarn

[flying-sheep]

Thanks! I actually did yarn set version canary but forgot to do yarn after before trying.

And forgot to search for CJS in the issues in addition to commonjs 🤦