Use require.resolve instead of the resolve package
#12439
Conversation
nicolo-ribaudo
added
PR: Internal 🏠
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33892/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d5b2aee:
|
| : (/* request */ r, { paths: [/* base */ b] }, M = require("module")) => { | ||
| let /* filename */ f = M._findPath(r, M._nodeModulePaths(b).concat(b)); | ||
| if (f) return f; | ||
| f = new Error(\`Cannot resolve module '\${r}'\`); |
There was a problem hiding this comment.
| f = new Error(\`Cannot resolve module '\${r}'\`); | |
| f = new Error(\`Cannot find module '\${r}'\`); |
To be consistent with the native require.resolve error message.
There was a problem hiding this comment.
Actually, I put resolve here otherwise all the tests where failing. I think Jest's require.resolve uses "resolve" in its error message.
There was a problem hiding this comment.
Not a blocker.
@SimenB Is it possible to test MODULE_NOT_FOUND error without changing the error message? We want to make assertion about the MODULE_NOT_FOUND error but Jest seems to assume such error means the test can not be run and all tests are failing.
There was a problem hiding this comment.
error from jest should match node, if it doesn't it's a bug
There was a problem hiding this comment.
@JLHwung The jest version we are using on Node 6 didn't always set MODULE_NOT_FOUND anyway, so we can't test it right now.
There was a problem hiding this comment.
I don't really follow here, but if Jest 26 does something wrong with MODULE_NOT_FOUND it would be great if you opened up an issue 🙂
There was a problem hiding this comment.
No don't worry! We are using Jest 24 on Node.js 6 and Jest 26 where it's supported. The bug was only with Jest 24, so we cannot rely on the fix but it is fixed in Jest 26!
**What's the problem this PR addresses?** - ~~#18768 started to ncc babel and thus it's version of resolve which breaks PnP support~~ Babel replaced `resolve` with the builtin `require.resolve` and a polyfill for older node versions in babel/babel#12439 which was upgraded in #20586 - `next` unnecessarily bundles the `resolve` package when `require.resolve` is builtin and can do the same job **How did you fix it?** - ~~Avoid running `resolve` through ncc~~ Added a test for #19334 (closes #19334) - Replace `resolve` with `require.resolve`
github-actions
bot
added
the
outdated
resolveThis PR fixes an incompatibility between Babel, PnP and Next.js.
The problem is that Next.js bundles Babel, and thus it bundles our
resolvedependency.resolvedoesn't support PnP, but Yarn patchesresolveat install time so that it works with PnP: this doesn't work ifresolveis inside of a bundle, because Yarn cannot detect it.(context: #12396)
This PR drops
"resolve", in favor orrequire.resolve(when supported) or a custom small polyfill when it's not.The other possible fix is that
resolvesupports PnP natively (rather than being patched at build-time), but it won't likely be implemented because it would unnecessarly ship a lot of code to consumers not using PnP (ref: browserify/resolve#199).babel.config.jsso that when building for Node.js 6 (and for releases) it inlines a small polyfill. The polyfill is the smallest I could think of, based on https://github.com/nodejs/node/blob/f223bba539c8dab131e443225a8695eba0fd5521/lib/module.js#L462 where_resolveLookupPathshas been replaced by what fits our usage.By doing so,
require.resolvewhen it supports thepathsoption.Note that customizing our build process to support older Node.js versions is something we already did: for example, before #12288 we were using a custom plugin to fix resolution of dynamic import paths in some Node.js versions.