v17.1.0 node resolution breaks pnpm-style symlinks

[mattfysh]

Version

v17.2.0

Platform

Darwin 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:23 PDT 2021; root:xnu-8019.41.5~1/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

--experimental-specifier-resolution=node with symlinks was working in v17.0.1 but broke in v17.1.0

Reproduction available here: https://github.com/mattfysh/resolve

Tested with multiple versions of node in docker using e.g. docker run --rm -it --entrypoint bash node:17.1.0

node v17.0.1 - works
node v17.1.0 - doesn't work
node v17.2.0 - doesn't work

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

The node resolution algorithm continues to work with pnpm-style symlinks

What do you see instead?

It can find the first package (a) but not it's dependency (b)

Additional information

Possibly related to #40510

[mscdex]

I'm assuming you mean v17.x.x instead of v7.x.x everywhere right?

[mattfysh]

Thanks @mscdex - updated

[Trott]

@nodejs/modules

[mattfysh]

To see how this behaviour is affecting pnpm, you can run the following:

mkdir test
cd test
pnpm init -f
pnpm i mem
node -e "import('mem')"
# works
node --experimental-specifier-resolution=node -e "import('mem')"
# ERR_MODULE_NOT_FOUND

The mem module can be found, but it's sub-dependency mimic-fn cannot. This is most likely due to the extensive use of symlinks in the pnpm node_modules structure

[GeoffreyBooth]

Are there reproduction steps that don’t involve pnpm? Like “create these files and folders, create this symlink, then run this”.

[mattfysh]

Hi @GeoffreyBooth did you see the repro in the original comment? This shows the problem occurring without pnpm, but it does mimic the pnpm node_modules structure

https://github.com/mattfysh/resolve

I only added the pnpm example for additional context but it is not required to reproduce the behaviour.

[bmeck]

It looks like the default resolver when using --experimental-specifier-resolution=node is not properly realpathing prior to storing in the module map (see different location of /mem/ vs /mem@9.0.1/ in the following):

bradley.farias@COMP-C02FN0TWML87 pnpm % NODE_DEBUG=esm node --experimental-specifier-resolution=node -e "import('mem')"
ESM 83537: Storing file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/mem/dist/index.js (implicit type) in ModuleMap
ESM 83537: Translating StandardModule file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/mem/dist/index.js
node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'mimic-fn' imported from /Users/bradley.farias/Documents/test/test/pnpm/node_modules/mem/dist/index.js
    at new NodeError (node:internal/errors:371:5)
    at packageResolve (node:internal/modules/esm/resolve:864:9)
    at moduleResolve (node:internal/modules/esm/resolve:910:18)
    at defaultResolve (node:internal/modules/esm/resolve:1005:11)
    at ESMLoader.resolve (node:internal/modules/esm/loader:475:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:245:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:79:40)
    at link (node:internal/modules/esm/module_job:78:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v17.1.0
bradley.farias@COMP-C02FN0TWML87 pnpm % NODE_DEBUG=esm node -e "import('mem')"
ESM 83605: Storing file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/mem@9.0.1/node_modules/mem/dist/index.js (implicit type) in ModuleMap
ESM 83605: Translating StandardModule file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/mem@9.0.1/node_modules/mem/dist/index.js
ESM 83605: Storing file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/mimic-fn@4.0.0/node_modules/mimic-fn/index.js (implicit type) in ModuleMap
ESM 83605: Storing file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/map-age-cleaner@0.1.3/node_modules/map-age-cleaner/dist/index.js (implicit type) in ModuleMap
ESM 83605: Translating CJSModule file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/map-age-cleaner@0.1.3/node_modules/map-age-cleaner/dist/index.js
ESM 83605: Translating StandardModule file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/mimic-fn@4.0.0/node_modules/mimic-fn/index.js
ESM 83605: Loading CJSModule file:///Users/bradley.farias/Documents/test/test/pnpm/node_modules/.pnpm/map-age-cleaner@0.1.3/node_modules/map-age-cleaner/dist/index.js

[mattfysh]

In the original bug report I've also included a link to the PR which I think may have introduced this bug. Just want to make sure this also hasn't been overlooked... #40510

[bmeck]

@mattfysh thanks, that made it real quick to find. https://github.com/nodejs/node/blame/32f7218388216b650bd27e0260401c60f41f73fd/lib/internal/modules/esm/resolve.js#L398-L408 got moved by that PR to occur after --experimental-specifier-resolution=node is applied, likely it just needs to be moved ahead of that bit of code. Unfortunately --experimental-specifier-resolution=node isn't actively developed anymore so I can't say this won't happen again but we can fix it this time.

[GeoffreyBooth]

cc @guybedford

[mattfysh]

Unfortunately --experimental-specifier-resolution=node isn't actively developed anymore

Got it, sounds like I'd be better off migrating to the explicit resolution mode? Is there a tool that I can run on my codebase to update import paths? Eg. import './dir' to import './dir/index.js'? Thanks 😀

[mainettis]

@mattfysh ctrl+shift+f on most gui IDEs with rgex pattern like (import '\.\/.+)?\/(?<!\.js)(?=';) and replace all with $1/index.js. I know vim and other cli editors also have a "replace all across path files" feature as well. 😀

Just ran into a similar issue trying to set up a new environment with version 17.3.1, yarn 3.x.x, mocha 9.1.4 and ts-node 10.x.x . While attempting to register ts-node in mocha, t seems to append a /node_module/<module> path to the mocha zip file path and throws ENOTDIR error.

I've worked around it using require.resolve('ts-node/register') method. But, I'm still playing around with it and investigating. I'm glad I found this.