Skip to content
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

Merged
merged 14 commits into from
Apr 21, 2020

Conversation

fisker
Copy link
Member

@fisker fisker commented Apr 17, 2020

This reverts #7508 & #7951

Fixes #8047

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@thorn0
Copy link
Member

thorn0 commented Apr 17, 2020

No, we shouldn't revert it. Let's just use it if require.resolve is broken. Otherwise we'll break #7407.

@fisker
Copy link
Member Author

fisker commented Apr 17, 2020

I missed that.

@fisker
Copy link
Member Author

fisker commented Apr 17, 2020

I need go out for couple hours now.

src/common/resolve.js Outdated Show resolved Hide resolved
src/common/resolve.js Outdated Show resolved Hide resolved
Coding on cellphone :)
@fisker

This comment has been minimized.

@fisker fisker changed the title Use resolve insteadof require.resolve Use resolve instead of require.resolve Apr 17, 2020
@fisker fisker changed the title Use resolve instead of require.resolve Use resolve if require.resolve is overide Apr 17, 2020
@fisker fisker marked this pull request as ready for review April 17, 2020 18:59
@fisker fisker changed the title Use resolve if require.resolve is overide Use resolve if require.resolve is overridden Apr 17, 2020
@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

@thorn0 I checked the index.js file in dist, on L24254

let {
  resolve: resolve$1
} = eval("require");

I remember this eval can't run in atom?

Link

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

eval('require').cache used in third-party.js because

"require.cache": "eval('require').cache",

@@ -2,11 +2,16 @@

let { resolve } = eval("require");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let { resolve } = eval("require");
let resolve = eval("require").resolve;

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

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) {

Copy link
Member

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.

Copy link
Member

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. 🤔

Copy link
Member Author

@fisker fisker Apr 20, 2020

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.

@thorn0
Copy link
Member

thorn0 commented Apr 20, 2020

@fisker I think everything is good here. Could you please re-check?

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

Everything should be good, thanks. Can you fix this #8072 (comment) , before you release new patch ?

@thorn0
Copy link
Member

thorn0 commented Apr 20, 2020

Is it broken? I thought that replacement happens for a reason.

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

I remember you add it ? I don't know if it can run in atom , I just saw it in third-party.js

Edit: not you , sorry .

@thorn0
Copy link
Member

thorn0 commented Apr 20, 2020

It wasn't me. I fixed only this thing: #7820

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

#5558 add it, if it's true that atom can't run eval , we need fix it.

@thorn0
Copy link
Member

thorn0 commented Apr 20, 2020

Okay. I'm downloading Atom. :D

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

Can you also check if it support new Function("return this")()?

I believe it's also used.

@fisker
Copy link
Member Author

fisker commented Apr 20, 2020

I'm going to sleep, good luck!

@fisker
Copy link
Member Author

fisker commented Apr 21, 2020

@thorn0 I tested in atom, both prettier files and prettier.config.js file can run eval.

@thorn0
Copy link
Member

thorn0 commented Apr 21, 2020

I haven't been able to reproduce problems with eval and new Function in Atom. However, I confirmed that the fallback from this PR works there.

@thorn0 thorn0 merged commit cfb28d6 into prettier:master Apr 21, 2020
@fisker
Copy link
Member Author

fisker commented Apr 21, 2020

And I logged console.log({global, require}) in our source, and played with it in console

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)}

global.require.resolve looks like the native require.resolve

@fisker fisker deleted the pr-7508-revert branch April 21, 2020 07:45
@thorn0
Copy link
Member

thorn0 commented Apr 21, 2020

It's not native. Module._resolveFilename that it calls has length === 2 and ignores options. Execute debugger; global.require.resolve('a', {paths:['b']}) to explore that function in the debugger.

thorn0 pushed a commit that referenced this pull request Apr 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: createRequire is not a function
2 participants