Use `resolve` if `require.resolve` is overridden by fisker · Pull Request #8072 · prettier/prettier

fisker · 2020-04-17T09:52:35Z

~~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 · 2020-04-17T09:54:43Z

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

fisker · 2020-04-17T09:56:41Z

I missed that.

fisker · 2020-04-17T10:30:40Z

I need go out for couple hours now.

src/common/resolve.js

Coding on cellphone :)

.github/workflows/prod-test.yml

fisker · 2020-04-20T11:00:19Z

@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 · 2020-04-20T11:00:19Z

@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 · 2020-04-20T11:13:23Z

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

prettier/scripts/build/config.js

Line 173 in ea8330a

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

thorn0 · 2020-04-20T13:05:24Z

src/common/resolve.js

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

 let { resolve } = eval("require");


Suggested change

let { resolve } = eval("require");

let resolve = eval("require").resolve;

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

I'm not sure, but global.require.resolve seems a good solution.

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

global.require exists only in the REPL.

Strangely. locally ESLint doesn't issue a warning for this line for me. 🤔

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 · 2020-04-20T16:52:41Z

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

src/common/resolve.js

fisker · 2020-04-20T17:08:17Z

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

thorn0 · 2020-04-20T17:18:40Z

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

fisker · 2020-04-20T17:20:35Z

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 · 2020-04-20T17:24:00Z

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

fisker · 2020-04-20T17:29:15Z

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

thorn0 · 2020-04-20T17:37:32Z

Okay. I'm downloading Atom. :D

fisker · 2020-04-20T17:48:10Z

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

I believe it's also used.

fisker · 2020-04-20T17:56:55Z

I'm going to sleep, good luck!

fisker · 2020-04-21T07:03:47Z

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

thorn0 · 2020-04-21T07:08:48Z

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.

fisker · 2020-04-21T07:10:51Z

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

thorn0 · 2020-04-21T07:56:31Z

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.

# Conflicts: # package.json

Use resolve insteadof require.resolve

c00689d

Use resolve as fallback

157e90e

fisker force-pushed the pr-7508-revert branch from f42a9e7 to 157e90e Compare April 17, 2020 10:05

Lazy load

d06c42e

thorn0 reviewed Apr 17, 2020

View reviewed changes

src/common/resolve.js Outdated Show resolved Hide resolved

thorn0 reviewed Apr 17, 2020

View reviewed changes

src/common/resolve.js Outdated Show resolved Hide resolved

Update resolve.js

8ab88f4

Coding on cellphone :)

thorn0 mentioned this pull request Apr 17, 2020

Uncaught TypeError: createRequire is not a function #8047

Closed

This comment has been minimized.

Sign in to view

Disable length check to run tests

cc574bb

fisker mentioned this pull request Apr 17, 2020

Refactor production tests #8073

Merged

fisker added 5 commits April 18, 2020 02:22

Merge branch 'master' into pr-7508-revert

34d881e

Try to test

eec17f5

istanbul ignore

b35f7cf

Comment

e13a469

Fix argument

bf56032

fisker force-pushed the pr-7508-revert branch from 2088155 to bf56032 Compare April 17, 2020 18:52

fisker changed the title ~~Use resolve insteadof require.resolve~~ Use resolve instead of require.resolve Apr 17, 2020

fisker changed the title ~~Use resolve instead of require.resolve~~ Use resolve if require.resolve is overide Apr 17, 2020

fisker marked this pull request as ready for review April 17, 2020 18:59

fisker changed the title ~~Use resolve if require.resolve is overide~~ Use resolve if require.resolve is overridden Apr 17, 2020

thorn0 reviewed Apr 17, 2020

View reviewed changes

.github/workflows/prod-test.yml Show resolved Hide resolved

Try upload coverage separately

0f0e8ce

thorn0 approved these changes Apr 18, 2020

View reviewed changes

thorn0 requested review from alexander-akait and sosukesuzuki April 20, 2020 09:08

thorn0 reviewed Apr 20, 2020

View reviewed changes

thorn0 added 2 commits April 20, 2020 16:05

Update src/common/resolve.js

7d741b2

add eslint-disable

61b3d4d

fisker commented Apr 20, 2020

View reviewed changes

src/common/resolve.js Show resolved Hide resolved

add comment

a5950e8

thorn0 force-pushed the pr-7508-revert branch from f502bd9 to a5950e8 Compare April 20, 2020 20:27

thorn0 merged commit cfb28d6 into prettier:master Apr 21, 2020

fisker deleted the pr-7508-revert branch April 21, 2020 07:45

thorn0 pushed a commit that referenced this pull request Apr 21, 2020

Use resolve if require.resolve is overridden (#8072)

6ad8a7f

# Conflicts: # package.json

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2021

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

Uh oh!

Conversation

fisker commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Apr 17, 2020

Uh oh!

fisker commented Apr 17, 2020

Uh oh!

fisker commented Apr 17, 2020

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

thorn0 Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

fisker Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

fisker Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

thorn0 Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

thorn0 Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

thorn0 Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

fisker Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Apr 20, 2020

Uh oh!

Uh oh!

fisker commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorn0 commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

thorn0 commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

fisker commented Apr 20, 2020

Uh oh!

fisker commented Apr 21, 2020

Uh oh!

thorn0 commented Apr 21, 2020

Uh oh!

fisker commented Apr 21, 2020

Uh oh!

thorn0 commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fisker commented Apr 17, 2020 •

edited

Loading

fisker Apr 20, 2020 •

edited

Loading

fisker commented Apr 20, 2020 •

edited

Loading

fisker commented Apr 20, 2020 •

edited

Loading