-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Use require.resolve
instead of the resolve
package
#12439
Use require.resolve
instead of the resolve
package
#12439
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error from jest should match node, if it doesn't it's a bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, good stuff 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good to me with a nit.
**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`
resolve
This PR fixes an incompatibility between Babel, PnP and Next.js.
The problem is that Next.js bundles Babel, and thus it bundles our
resolve
dependency.resolve
doesn't support PnP, but Yarn patchesresolve
at install time so that it works with PnP: this doesn't work ifresolve
is 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
resolve
supports 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.js
so 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_resolveLookupPaths
has been replaced by what fits our usage.By doing so,
require.resolve
when it supports thepaths
option.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.