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
Remove resolve
package
#7508
Remove resolve
package
#7508
Conversation
76dac9b
to
2e121ab
Compare
src/common/load-plugins.js
Outdated
const externalManualLoadPluginInfos = externalPluginNames.map(pluginName => ({ | ||
name: pluginName, | ||
requirePath: eval("require").resolve(pluginName, { paths: [process.cwd()] }) | ||
})); |
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.
This part might different with old one.
If pluginName = 'my-plugin'
not pluginName = './my-plugin'
, result might different, will look for my-plugin
package, insteadof ./my-plugin
, but no tests cover it.
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.
We should keep the existing behavior. I'd like to understand though what was the original motivation for using this module instead of require.resolve
. Any ideas?
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.
https://github.com/prettier/prettier/pull/3536/files .
require.resolve
supports options
until v8.9.0
https://nodejs.org/api/modules.html#modules_require_resolve_request_options
I think that's the reason
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.
Logic of this part has restored.
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.
Can you please add a test for that case?
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.
How important is it to support the old behavior in the new release (#6888)?
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.
This should take a while, I don't know where pluginName come from yet.
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.
This line seems mean to test this, but did't catch it.
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 add a print
console.log(
JSON.stringify(
{
pluginName,
"with-path.resolve": eval("require").resolve(
path.resolve(process.cwd(), pluginName)
),
"with-require.resolve(..., options)": eval("require").resolve(
pluginName,
{
paths: [process.cwd()]
}
)
},
null,
2
)
);
Got same result on ./automatic/node_modules/prettier-plugin-bar
and automatic/node_modules/prettier-plugin-bar
, not sure where this .js
lost.js
not lost, there is another test, but same result with .js
{
"pluginName": "./automatic/node_modules/prettier-plugin-bar",
"with-path.resolve": "<repo>\\prettier\\tests_integration\\plugins\\automatic\\node_modules\\prettier-plugin-bar\\index.js",
"with-require.resolve(..., options)": "<repo>\\prettier\\tests_integration\\plugins\\automatic\\node_modules\\prettier-plugin-bar\\index.js"
}
{
"pluginName": "automatic/node_modules/prettier-plugin-bar",
"with-path.resolve": "<repo>\\prettier\\tests_integration\\plugins\\automatic\\node_modules\\prettier-plugin-bar\\index.js",
"with-require.resolve(..., options)": "<repo>\\prettier\\tests_integration\\plugins\\automatic\\node_modules\\prettier-plugin-bar\\index.js"
}
But I ran on cmd, this failed as expected
<repo>\prettier\tests_integration\plugins>node
Welcome to Node.js v13.5.0.
Type ".help" for more information.
> require.resolve("automatic/node_modules/prettier-plugin-bar")
Uncaught Error: Cannot find module 'automatic/node_modules/prettier-plugin-bar'
Require stack:
- <repl>
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:966:17)
at Function.resolve (internal/modules/cjs/helpers.js:78:19) {
code: 'MODULE_NOT_FOUND',
requireStack: [ '<repl>' ]
}
> require.resolve("./automatic/node_modules/prettier-plugin-bar")
'<repo>\\prettier\\tests_integration\\plugins\\automatic\\node_modules\\prettier-plugin-bar\\index.js'
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.
@thorn0 have a look
I found something interesting, I print the "(moduleName, options) =>\n this._requireResolve(from.filename, moduleName, options)" But, on console $ node -p "require.resolve.toString()"
function resolve(request, options) {
validateString(request, 'request');
return Module._resolveFilename(request, mod, false, options);
} They are different So Jest not using the node.js require ???? Didn't find any docs about that. |
Test that can catch this error added #7517 |
Jest implements |
Is this documented somewhere? |
@SimenB yes, I figured, and there is a bug |
I don't think so. |
requirePath = eval("require").resolve( | ||
path.resolve(process.cwd(), pluginName) | ||
); | ||
} catch (_) { | ||
// try node modules | ||
requirePath = resolve.sync(pluginName, { basedir: process.cwd() }); | ||
requirePath = eval("require").resolve(pluginName, { | ||
paths: [process.cwd()] | ||
}); | ||
} |
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've merged #7517 into this one, to catch that error, remove this try
part and run
yarn test tests_integration/__tests__/plugin-resolution.js
@SimenB You can check my comment #7508 (comment) , If you want I can show you how to find the difference from jest |
I think it would be ideal if someone could do this in Q&A style on Stack Overflow. It would also help |
If you could open up an issue at fb/jest with a small reproduction that would be wonderful yeah |
@SimenB Let me try first on my machine |
checkout this repo https://github.com/fisker/jest-require-resolve-bug If you confirm that or something I did wrong, let me know here first, or in that repo. |
It took me a little while to understand the difference between I think it would also really help users in the future if someone could document what Jest does to the require statements and why. I am also wondering if Jest would do something similar to ES6 import statements. |
Yeah, that's a bug. Seems like we're missing something in the implementation from jestjs/jest#6471. Something about making things relative while they're not seems off. Could you open up an issue with it? @brodybits there should be no observable differences between Jest's |
Seeing as EDIT: Or use |
@SimenB Doesn't matter anymore, new test code won't relay |
I'm not talking about test code, I'm talking about loading user plugins, which is where you use the From the OP:
It cannot, it's broken on node 10 for the use case you want to use it. I'll unsubscribe since it doesn't really impact me (I don't use custom plugins), and I don't really have any interest in discussing this. Thanks for finding the inconsistency in Jest's implementation though! 😀 We'll replicate the implementation in the version of node we're running on (at least that's the current plan) |
Actually the node 10 behavior is more we want, when resolve plugin We found that problem is because I tried to remove |
Might fix #7407 if |
f5a82d9
to
259a8f6
Compare
259a8f6
to
dca0f2e
Compare
I think
require.resolve
can do the job.Tests from separate PR #7517
docs/
directory)changelog_unreleased/*/pr-XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨