-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 resolve
if require.resolve
is overridden
#8072
Conversation
No, we shouldn't revert it. Let's just use it if |
I missed that. |
I need go out for couple hours now. |
Coding on cellphone :)
This comment has been minimized.
This comment has been minimized.
resolve
insteadof require.resolve
resolve
instead of require.resolve
resolve
instead of require.resolve
resolve
if require.resolve
is overide
resolve
if require.resolve
is overideresolve
if require.resolve
is overridden
prettier/scripts/build/config.js Line 173 in ea8330a
|
src/common/resolve.js
Outdated
@@ -2,11 +2,16 @@ | |||
|
|||
let { resolve } = eval("require"); |
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.
let { resolve } = eval("require"); | |
let resolve = eval("require").resolve; |
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 tried this, won't work , this will replaced by babel https://github.com/prettier/prettier/blob/master/scripts/build/babel-plugins/transform-custom-require.js
to require.resolve
, then rollup replace it to commonJsRequire.resolve
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'm not sure, but global.require.resolve
seems a good solution.
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.
Are you sure? I tested it too locally and it stays this way in dist/index.js
:
let resolve$1 = require.revolve; // In the VS Code and Atom extension `require` is overridden and `require.resolve` doesn't support the 2nd argument.
if (resolve$1.length === 1 || process.env.PRETTIER_FALLBACK_RESOLVE) {
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.
global.require
exists only in the REPL.
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.
Strangely. locally ESLint doesn't issue a warning for this line for me. 🤔
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.
Hmm, I guess I tried let { resolve } = require;
andlet resolve = require.resolve;
.
Eslint didn't warn on my machine either, we need add comments and disable the rule on it anyway.
@fisker I think everything is good here. Could you please re-check? |
Everything should be good, thanks. Can you fix this #8072 (comment) , before you release new patch ? |
Is it broken? I thought that replacement happens for a reason. |
I remember you add it ? I don't know if it can run in atom , I just saw it in Edit: not you , sorry . |
It wasn't me. I fixed only this thing: #7820 |
#5558 add it, if it's true that atom can't run eval , we need fix it. |
Okay. I'm downloading Atom. :D |
Can you also check if it support I believe it's also used. |
I'm going to sleep, good luck! |
@thorn0 I tested in atom, both prettier files and |
I haven't been able to reproduce problems with |
And I logged temp2.global.require
ƒ require(path) {
try {
exports.requireDepth += 1;
return mod.require(path);
} finally {
exports.requireDepth -= 1;
}
}
temp2.require
ƒ require(e){return n.require(e)}
temp2.global.require.resolve
ƒ resolve(request, options) {
if (typeof request !== 'string') {
throw new ERR_INVALID_ARG_TYPE('request', 'string', request);
}
return Module._resolveFilename(request, mod, false, op…
temp2.require.resolve
ƒ (e){return get_Module()._resolveFilename(e,n)}
|
It's not native. |
This reverts #7508 & #7951Fixes #8047
docs/
directory)changelog_unreleased/*/pr-XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨