Remove `resolve` package by fisker · Pull Request #7508 · prettier/prettier

fisker · 2020-02-01T08:26:44Z

I think require.resolve can do the job.

Tests from separate PR #7517

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.

fisker · 2020-02-01T20:15:41Z

src/common/load-plugins.js

+  const externalManualLoadPluginInfos = externalPluginNames.map(pluginName => ({
+    name: pluginName,
+    requirePath: eval("require").resolve(pluginName, { paths: [process.cwd()] })
+  }));


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.

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?

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

Logic of this part has restored.

Can you please add a test for that case?

How important is it to support the old behavior in the new release (#6888)?

This should take a while, I don't know where pluginName come from yet.

This line seems mean to test this, but did't catch it.

https://github.com/prettier/prettier/blob/master/tests_integration/__tests__/plugin-resolution.js#L148

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 .js lost~~ this .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'

@thorn0 have a look

fisker · 2020-02-02T17:22:20Z

I found something interesting, I print the require.resolve.toString() on test,

"(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.

fisker · 2020-02-02T17:23:34Z

Found it in code

https://github.com/facebook/jest/blob/c9127bfec38882f02abed4bf41514f1843708ecb/packages/jest-runtime/src/index.ts#L1059

fisker · 2020-02-02T18:20:18Z

Test that can catch this error added #7517

SimenB · 2020-02-02T18:25:42Z

Jest implements require on its own to support transpilation and mocks

brody4hire · 2020-02-02T18:27:43Z

Jest implements require on its own to support transpilation and mocks

Is this documented somewhere?

fisker · 2020-02-02T18:28:47Z

@SimenB yes, I figured, and there is a bug require.resolve('a-path/to/file') should try resolve a-path module not a-path/to/file.js file

SimenB · 2020-02-02T18:29:27Z

I don't think so. require's implementation is an implementation detail, same as for bundlers

fisker · 2020-02-02T18:31:34Z

src/common/load-plugins.js

+      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()]
+      });
    }


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

fisker · 2020-02-02T18:33:56Z

@SimenB You can check my comment #7508 (comment) ,

If you want I can show you how to find the difference from jest require.resolve and node.js require.resolve .

brody4hire · 2020-02-02T18:37:43Z

If you want I can show you how to find the difference from jest require.resolve and node.js require.resolve .

I think it would be ideal if someone could do this in Q&A style on Stack Overflow. It would also help require.resolve newbies such as myself if someone can describe how this PR was able to overcome this limitation.

SimenB · 2020-02-02T18:48:52Z

@SimenB You can check my comment #7508 (comment) ,

If you want I can show you how to find the difference from jest require.resolve and node.js require.resolve .

If you could open up an issue at fb/jest with a small reproduction that would be wonderful yeah

fisker · 2020-02-02T18:49:36Z

@SimenB Let me try first on my machine

fisker · 2020-02-02T19:01:38Z

@SimenB

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.

brody4hire · 2020-02-02T19:29:47Z

https://github.com/fisker/jest-require-resolve-bug

It took me a little while to understand the difference between require.resolve on Node.js vs Jest. The difference I found is that require.resolve('foo/node_modules/bar', {paths: [process.cwd()]}) throws an error in Node.js but succeeds in Jest. Looks like a subtle bug in Jest.

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.

SimenB · 2020-02-02T22:23:05Z

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 require and node's implementations (by invoking the functions, not toString etc) beyond options passed to Jest that change it, like moduleNameMapper and code transforms. So nothing really to document. It'll be the same with ESM when that's implemented

SimenB · 2020-02-04T13:17:43Z

Seeing as require.resolve with paths is broken in Node 10 (fixed in 12: nodejs/node#23683) I'd suggest to not land this until you also drop node 10.

EDIT: Or use createRequire{FromPath}(fromWhereIWannaLook).resolve instead. Note that while createRequire has landed on Jest master, it has not been released. I'd still suggest sticking with the resolve package, but your call 🙂

fisker · 2020-02-04T13:37:49Z

@SimenB Doesn't matter anymore, new test code won't relay require.resolve to throw

SimenB · 2020-02-04T13:52:43Z

I'm not talking about test code, I'm talking about loading user plugins, which is where you use the resolve package today. Using require.resolve('asdasd', {paths: []}) when you know it's broken on Node 10 seems irresponsible to me, especially if it's just about avoiding a single dependency and not some feature resolve doesn't support or resolve messing up browser builds or some such.

From the OP:

I think require.resolve can do the job.

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)

fisker · 2020-02-04T14:00:02Z

Actually the node 10 behavior is more we want, when resolve plugin foo, we try <CWD>/foo first. So node 10 broken require.resolve doesn't effect us, we are not breaking anything.

We found that problem is because I tried to remove <CWD>/foo first, but didn't catch a error. So we improved the test, make sure we are not changing the logic.

thorn0 · 2020-02-04T14:14:14Z

Might fix #7407 if require.resolve uses NODE_PATH.

fisker force-pushed the resolve-to-require-resolve branch 2 times, most recently from 76dac9b to 2e121ab Compare February 1, 2020 11:52

fisker commented Feb 1, 2020

View reviewed changes

brody4hire mentioned this pull request Feb 2, 2020

[RFC DRAFT WIP ...] refactor eval("require") workaround hack #7515

Closed

3 tasks

brody4hire mentioned this pull request Feb 2, 2020

Improve plugin-resolution test #7517

Merged

4 tasks

fisker commented Feb 2, 2020

View reviewed changes

fisker mentioned this pull request Feb 3, 2020

require.resolve with options seems have different result in some case jestjs/jest#9502

Closed

thorn0 approved these changes Feb 3, 2020

View reviewed changes

alexander-akait approved these changes Feb 18, 2020

View reviewed changes

thorn0 force-pushed the resolve-to-require-resolve branch from f5a82d9 to 259a8f6 Compare February 25, 2020 00:17

Remove resolve package

48fe832

fisker added 6 commits February 25, 2020 02:37

Remove usage in resolve-config.js

3c86e55

fix config resolve

6b30592

Fix snapshot

5bb80c2

use eval('require')

4affd39

Fix error message

2961442

restore logic

dca0f2e

thorn0 force-pushed the resolve-to-require-resolve branch from 259a8f6 to dca0f2e Compare February 25, 2020 00:38

thorn0 merged commit 129fcb6 into prettier:next Feb 25, 2020

fisker deleted the resolve-to-require-resolve branch February 25, 2020 08:40

thorn0 mentioned this pull request Mar 23, 2020

Document how to integrate in pre-commit with plugins #7407

Closed

fisker mentioned this pull request Apr 3, 2020

Potential Breaking Change in resolveConfig #7949

Closed

fisker mentioned this pull request Apr 17, 2020

Use resolve if require.resolve is overridden #8072

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2021

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Feb 1, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Feb 2, 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!

Choose a reason for hiding this comment

Uh oh!

fisker commented Feb 2, 2020

Uh oh!

fisker commented Feb 2, 2020

Uh oh!

fisker commented Feb 2, 2020

Uh oh!

SimenB commented Feb 2, 2020

Uh oh!

brody4hire commented Feb 2, 2020

Uh oh!

fisker commented Feb 2, 2020

Uh oh!

SimenB commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker Feb 2, 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!

fisker commented Feb 2, 2020

Uh oh!

brody4hire commented Feb 2, 2020

Uh oh!

SimenB commented Feb 2, 2020

Uh oh!

fisker commented Feb 2, 2020

Uh oh!

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

Uh oh!

Uh oh!

brody4hire commented Feb 2, 2020

Uh oh!

SimenB commented Feb 2, 2020

Uh oh!

SimenB commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Feb 4, 2020

Uh oh!

SimenB commented Feb 4, 2020

Uh oh!

fisker commented Feb 4, 2020

Uh oh!

thorn0 commented Feb 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

fisker commented Feb 1, 2020 •

edited

Loading

fisker Feb 1, 2020 •

edited

Loading

fisker Feb 2, 2020 •

edited

Loading

SimenB commented Feb 2, 2020 •

edited

Loading

fisker Feb 2, 2020 •

edited

Loading

fisker commented Feb 2, 2020 •

edited

Loading

SimenB commented Feb 4, 2020 •

edited

Loading