Potential Breaking Change in resolveConfig

[ntotten]

It seems that 2.0 may have introduced an unintended breaking change in the resolveConfig function. This function is used in the Prettier VS Code extension here: https://github.com/prettier/prettier-vscode/blob/master/src/ConfigResolver.ts#L99

Since upgrading to 2.0 we are now seeing errors when trying to resolve configs:

Error: Cannot find module 'prettier-config-greenelab' from 'c:\Users\Vincent\Desktop\adage-frontend'
	at Function.Module._resolveFilename (internal/modules/cjs/loader.js:717:15)
	at Function.n.resolve (c:\Program Files\Microsoft VS Code\resources\app\out\vs\loader.js:15:584)
	at Object.transform (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\index.js:23283:40)
	at run (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11472:51)
	at async cacheWrapper (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11300:20)
	at async cacheWrapper (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11300:20)
	at async cacheWrapper (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11300:20)
	at async cacheWrapper (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11300:20)
	at async cacheWrapper (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11300:20)
	at async Explorer.search (c:\Users\Vincent\.vscode\extensions\esbenp.prettier-vscode-4.0.0\node_modules\prettier\third-party.js:11457:22)
	at async Promise.all (index 0)

For repro steps and more details see: prettier/prettier-vscode#1289

One thing to note, is that everything behaves as expected from the CLI which is odd.

[ntotten]

@lipis Opened an issue in prettier repo to track.

[fisker]

@ntotten Can you try run node -p "require('prettier-config-greenelab')" in this dir 'c:\Users\Vincent\Desktop\adage-frontend' ?

I didn't see this package on npm , please check if your package.json in prettier-config-greenelab and the one in your project root has type :"module"

[fisker]

Anyway we should change this line

prettier/src/config/resolve-config.js

Line 27 in 4ac088c

error.message = `Cannot find module '${result.config}' from '${dir}'`;

to show the original message , I think I break this in #7508 . 😄

[ntotten]

Running node -p "require('prettier-config-greenelab')" seems to work as expected. The module isn't in NPM, its just a sample the OP of our bug created, you can find it here: github.com/greenelab/prettier-config-greenelab

[vincerubinetti]

Hi folks, I'm the one who originally posted the downstream issue on VSCode Prettier. That config is not on npm because I didn't feel like publishing and maintaining a package just for a small lint config file. You can install it right from the git repo:

https://github.com/greenelab/prettier-config-greenelab

And that's an actual config I've been using, didn't create it just for the bug.

Maybe the bug is even specifically related to packages installed from github repos? I can't say.

[ntotten]

It looks like the issue is that prettier removed support for config for files with just the string content:

In .prettierrc.yaml this no longer works:

"prettier-config-greenelab"

However, this does work:

prettier: "prettier-config-greenelab"

I think as long as prettier returns a better error message and the breaking change is noted, this is probably okay - assuming it was deliberate.

[vincerubinetti]

@ntotten I just tried that, and it worked, even in VSCode Prettier v 4.0.0.

Here is the relevant docs section that needs to be updated:
https://prettier.io/docs/en/configuration.html#sharing-configurations

[fisker]

No we didn't, I'll check it later.

[thorn0]

@fisker Looks like require.resolve doesn't work in VS Code extensions the way it works in Node 8.9.0+. require.resolve.length returns 1.

[ntotten]

Ah, this is probably because the extension is webpacked.

The extension handles this in our module loader: https://github.com/prettier/prettier-vscode/blob/master/src/ModuleResolver.ts#L263-L265

[fisker]

but

prettier: "prettier-config-greenelab"

this work ? This also use require.resolve

[fisker]

@ntotten where did you put this prettier: "prettier-config-greenelab"?

[thorn0]

@fisker Are you sure? I thought cosmiconfig deals with that.

[fisker]

@thorn0 I'm sure, if it's in package.json.

[ntotten]

I think the reason that it seems to work when it is set to prettier: "prettier-config-greenelab" is because resolveConfig then just returns:

{ "prettier": "prettier-config-greenelab" }

Then the extension passes this config back to prettier and then prettier resolves the full config. However, in the case we pass just "prettier-config-greenelab" it looks like resolveConfig attempts to load the config from the module and should return the actually config value:

{ printWidth: 80,
  tabWidth: 2,
  useTabs: false,
  semi: true,
  singleQuote: true,
  quoteProps: 'consistent',
  jsxSingleQuote: true,
  trailingComma: 'none',
  bracketSpacing: true,
  jsxBracketSameLine: false,
  arrowParens: 'always',
  requirePragma: false,
  insertPragma: false,
  proseWrap: 'always',
  htmlWhitespaceSensitivity: 'css',
  endOfLine: 'lf' }

I am not sure why the module resolution seems to work in one case but not the other though.

[fisker]

Alright, I thought you put that in package.json, so should be the require.resolve problem, I misunderstood that.

[ntotten]

Seems cosmiconfig doesn't handle webpack: cosmiconfig/cosmiconfig#222

[thorn0]

@ntotten It doesn't look like your loader overrides require.resolve. Or am I wrong? The problem seems to be the fact that in Node 8.9.0+ require.resolve accepts a second argument, which Prettier 2.0 uses, but in the extension require.resolve seems to not support that.

[fisker]

It's not about cosmiconfig, it gives us the string, which is expected, we require it on our side.

[fisker]

I guess everyone has their own require? First jest, now vscode..

[ntotten]

The require is from webpack: https://webpack.js.org/api/module-methods/#requireresolve

But it doesn't look like it supports the options parameter.

[fisker]

No, there is only one argument, should be two.

[thorn0]

@ntotten I there a way to have the original require.resolve there? The loading of plugins relies on it too, not only configs.

[ntotten]

@thorn0 I am not entirely sure to be honest. The other thing I am a bit confused about is why this also happens when I am running the extension in debug mode - which doesn't get webpacked and should have the original require.resolve.

[ntotten]

Actually, i think we are on the wrong track. We should always be using the regular require.

      typeof __webpack_require__ === "function"
        ? __non_webpack_require__
        : require;

That will return the default global require regardless of if we are webpacked or not. See: https://webpack.js.org/api/module-variables/#__non_webpack_require__-webpack-specific

But require.resolve.length === 1 so maybe this is actually something with vscode or electron.

[fisker]

@ntotten Is there a way you can replace require.resolve with something like resolve-from.

[ntotten]

@fisker I could, but I would need some way to pass my resolver to prettier.

[ntotten]

It seems to be a known issue in VS Code. It is due to webpack's restriction on require.resolve. microsoft/vscode#78894

[thorn0]

I have a fix: require('module').createRequire(dir).resolve.length === 2

[thorn0]

@ntotten Should we fix this on Prettier's side or in the extension (by passing a require instance created with require('module').createRequire in ModuleResolver)?

[ntotten]

@thorn0 It's up to you, given that I already have to deal with this in several places in the extension, I'm fine passing in an instance of require, but it also might be cleaner just to do it in prettier so there isn't any public API to deal with.

[thorn0]

Okay, I'm about to submit a PR.