Open arcanis opened 5 years ago
:wave: I had a few questions what Plug n' Play will look like with ESLint after https://github.com/eslint/eslint/pull/11388 is merged, and whether the ESLint team might be able to make it better, and it seems like this would be a good place to ask.
With https://github.com/eslint/eslint/pull/11388, ESLint will load plugin relative to its "CWD" (note that the someone invoking ESLint can set the "CWD" that it uses to something other than process.cwd()
). This should allow packages like react-scripts
to use it reliably, by setting its "CWD" to the location of their own package. (We might add a specific flag for this to avoid other inconvenient side-effects of changing the CWD that ESLint uses.)
node_modules
folder, and Node scripts either need to be invoked with yarn
or with something like -r .pnp.js
. So if an editor integration needs to load an ESLint plugin to lint a user's project, but it isn't aware of PnP, does it just fail?If a user has a create-react-app
project and is using an ESLint editor integration, the integration typically won't know that it's supposed to use a different CWD. As a result, it would probably load ESLint plugins relative to process.cwd()
, which is typically in the user's project. Would this result in missing-plugin errors?
(This seems like it would be a broader issue with editor integrations not having access to tools/config files when using things like create-react-app
, although it would be good to know in advance if this is going to cause a problem so that we can work around it if needed.)
👋 !
How do editor integrations typically work with PnP? My understanding is that when Plug n' Play is in use, there is no
node_modules
folder, and Node scripts either need to be invoked withyarn
or with something like-r .pnp.js
. So if an editor integration needs to load an ESLint plugin to lint a user's project, but it isn't aware of PnP, does it just fail?
If the editors invoke ESLint through its CLI interface they'll just have to invoke yarn eslint
instead of ./node_modules/.bin/eslint
and it will just work, regardless of whether the project uses PnP or not. I could imagine a plugin generating a compatibility wrapper in node_modules/.bin
each time a package with binary scripts is added to the dependency tree, but ideally it shouldn't be needed.
If the editors do more exotic things like running an internal instance of ESLint that require()
the plugins shipped with the project it might cause issues indeed, since they won't know where to look for them (plus they won't be able to open them without further processing, since they are not unpacked). Do you think that will be a problem? On the top of my head I would think this pattern shouldn't occur often, since the plugins might not be compatible with the internal ESLint version anyway - so calling the local binary installed with the project would seem the safest path.
If a user has a
create-react-app
project and is using an ESLint editor integration, the integration typically won't know that it's supposed to use a different CWD. As a result, it would probably load ESLint plugins relative toprocess.cwd()
, which is typically in the user's project. Would this result in missing-plugin errors?
Based on what I understand from https://github.com/eslint/eslint/pull/11388 it seems like react-scripts
would tell ESLint that its lint rules should be resolved from its own cwd
("and other packages relative to where they're specified in a config"), is that correct? If so it looks like it should work fine.
If the editors invoke ESLint through its CLI interface they'll just have to invoke
yarn eslint
instead of./node_modules/.bin/eslint
and it will just work, regardless of whether the project uses PnP or not.
Thanks, that makes sense.
When using something like create-react-app
, it seems like this could be a problem because yarn eslint
wouldn't find anything (since react-scripts
installs eslint
, but the end user does not). However, I think some editor integrations bundle their own version of ESLint to get around this issue.
Based on what I understand from eslint/eslint#11388 it seems like
react-scripts
would tell ESLint that its lint rules should be resolved from its owncwd
("and other packages relative to where they're specified in a config"), is that correct? If so it looks like it should work fine.
At the moment, only the user/tool that actually invokes ESLint can tell it where to find rules. (This avoids the possibility of having multiple parties give ESLint conflicting information.) Whoever invokes ESLint is required to install plugins (packages containing rules) themselves. So it seems like editor integrations would have trouble with this case, since they try to invoke ESLint but wouldn't have access to the info from react-scripts
that describes how ESLint should be invoked. (This is also a problem for some other CLI flags, and isn't specific to package-loading. I suppose the general solution is to put more information in config files so that editor integrations don't need to deal with that sort of thing.)
Thanks, I think that answers my questions -- I might propose some separate changes to ESLint based on this info.
If the editors invoke ESLint through its CLI interface they'll just have to invoke yarn eslint instead of ./node_modules/.bin/eslint and it will just work, regardless of whether the project uses PnP or not.
Following this advice, I configured vscode-eslint to use yarn as package manager to use eslint. I'm using a fresh CRA install.
./vscode/settings.json
{
"eslint.packageManager": "yarn"
}
Unfortunately, it doesn't seems to be enough for vscode-eslint to work properly:
[Info - 17:21:43] ESLint server stopped.
[Info - 17:21:43] ESLint server running in node v10.2.0
[Info - 17:21:43] ESLint server is running.
[Info - 17:21:45]
Failed to load the ESLint library for the document /home/mbeaudru/projects/my-awesome-project/src/App.js
To use ESLint please install eslint by running yarn add eslint in the workspace folder find-this-song
or globally using 'yarn global add eslint'. You need to reopen the workspace after installing eslint.
Alternatively you can disable ESLint for the workspace folder find-this-song by executing the 'Disable ESLint' command.
Is there something I'm missing to make it work ? I also tried to unplug eslint and added the eslint bin in NODE_PATH but it didn't worked as well.
@mbeaudru - i think if you dig into vscode-eslint
plugin's code, setting the packageManager
property is only useful in telling it where to find a globally installed version of ESLint.
I believe this issue to be the problem - VSCode's language server implementation isn't PnP aware, which their ESLint plugin uses. There might be a simple workaround, but I'm unsure - I don't actively use VSCode.
@not-an-aardvark - I have a scenario where I need the CLI flag --resolve-plugins-relative-to
to get PnP working inside a monorepo, which is a non-issue besides that I can't pass it to various wrappers ex. prettier-eslint-cli
. Is this configurable, or do I need to write my own wrapper of the ESLint Node.js API & should open an feature request/PR on who's repository?
@arcanis - for context, I've got a project that contains a workspace named @meatwallace/eslint-config
, and I want to have an .eslintrc
in the root of the project that extends it. My root package.json
depends on "@meatwallace/eslint-config": "workspace:*"
- is this the correct approach in v2?
I'm not sure I understand the question, but it is configurable using the --resolve-plugins-relative-to
flag or the corresponding resolvePluginsRelativeTo
Node.js API option.
I have the issue when extending config written like this https://github.com/ringcentral/ringcentral-javascript/blob/master/eslint-config-ringcentral-typescript/src/index.js ESLint tries to resolve from CWD and fails, obviously.
@not-an-aardvark:
At the moment, only the user/tool that actually invokes ESLint can tell it where to find rules. (This avoids the possibility of having multiple parties give ESLint conflicting information.) Whoever invokes ESLint is required to install plugins (packages containing rules) themselves.
This defeats the benefits of installing third party configs and NPM basic principles. I tried to specify all dependencies of the entire eslint config/plugin tree, it's quite nasty. Moreover, if any config down the line will change I will have to update my "fix".
Without PnP everything is just lifted to node_modules
so plugin/config authors were able to list whatever they wanted, everything came out flat.
I haven't found a way to rewrite something like extends: 'plugin:jsx-a11y/recommended'
into require.resolve
call because recommended
is just a property in https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/src/index.js#L43, and extends
only accept strings.
I also tried to explicitly list plugins in eslint-config-ringcentral-typescript
like so: plugins: [require.resolve('eslint-plugin-import'), ...]
but no luck either. I guess that's more of a question to ESLint, how to let it know where the plugin resides.
Been lurking, but thought I may as well comment as well, which is hopefully not jut a me-too...
Have also played with the yarn 2 conversion, and the last hurdle is the eslint configs. (In my specific case there is a base package with all the rules and each project in the GH org uses exactly the same config) It certainly would not make sense to have each and every repo install all the eslint deps in my case, it becomes a rabbit hole and then also needs to be maintained down the line in multiple places instead of having each and every repo standard.
There are some extends that I managed to convert via resolver, e.g. https://github.com/polkadot-js/dev/blob/83892ce57440d56fb7ac14c06549dc9399c84c2d/packages/dev/config/eslint.js#L15
(here the resolver import is just a small wrapper around require.resolve that take take string, string[] or [string, options] and just maps it - it helped in the case of babel as well keeping it kinda neat)
With some hacking and starting to extract rules form the packages extended (and including them manually, from source), can get to the point where the extends part actually more-or-less work (or at least doesn't complain anymore), although obviously this is not quite what the "extends" intended.
However, when it gets to the plugin definitions, it expects a string[] and there is no resolver that can be used there.
TL;DR For shared config, eslint indeed has "some" issues as indicated in the documentation :)
It would be great if at least ESLint would allow to explicitly declare where to look for the plugins, kinda like Webpack aliases:
// top level eslintrc
alias: {
'plugin:prettier': require.resolve('eslint-plugin-prettier')
}
This is still ugly, but at least it will make it work.
Ideally authors of configs/plugins should simply use require.resolve
everywhere to make sure they resolve their dependencies properly and not try to lift this knowledge up to end-users.
I have found an interesting solution which I'd like to put here: https://github.com/yarnpkg/pnp-sample-app/blob/master/scripts/eslint-resolver.js
Usage: https://github.com/yarnpkg/pnp-sample-app/blob/master/.eslintrc.js#L22
Also I had to install all dependencies and sub-dependencies of ESLint configs that I use, which is horrific to see:
{
"devDependencies": {
"@typescript-eslint/eslint-plugin": "^2.11.0",
"@typescript-eslint/parser": "^2.11.0",
"eslint-config-prettier": "^6.7.0",
"eslint-config-react-app": "^5.1.0",
"eslint-config-ringcentral": "^3.0.0",
"eslint-import-resolver-node": "^0.3.3",
"eslint-plugin-flowtype": "^4.5.2",
"eslint-plugin-import": "^2.19.1",
"eslint-plugin-jsx-a11y": "^6.2.3",
"eslint-plugin-prettier": "^3.1.1",
"eslint-plugin-react": "^7.17.0",
"eslint-plugin-react-hooks": "^2.3.0",
"eslint-plugin-ringcentral": "^1.0.0",
"eslint-plugin-sonarjs": "^0.5.0",
"prettier": "^1.19.1"
},
}
Instead of just
{
"dependencies": {
"eslint-config-ringcentral-typescript": "3.0.0",
}
}
as it used to be without PNP (not the issue of PNP, blame ESLint resolution algorithm).
I can't imagine how fragile this is and how much pain it will cause taking into account the number of projects that use company-wide config...
Is it possible to disable PnP? We can't continue with Berry if our ESlint falls over.
@Robula you can use https://github.com/yarnpkg/berry/tree/master/packages/plugin-node-modules which will place files the same way as traditional NPM/Yarn would.
The problem is that then you won't get any performance benefits...
And plus, like I said earlier, I was able to use ESLint with PNP, the only problem is how dirty it is (all those dependencies lifted up to application level).
Thanks @kirill-konshin I may have to revert to using that or go back to Yarn 1. Are the performance benefits really worth it when you need 100+ lines of packageExtension
?
I'm currently trying to move our monorepo to Berry I am trying to get everything to play nicely with PnP. Here's what I got so far (many more to go):
"@storybook/addon-a11y@*":
peerDependencies:
react-dom: "*"
regenerator-runtime: "*"
"@storybook/addon-actions@*":
peerDependencies:
"@storybook/core": "*"
react-dom: "*"
regenerator-runtime: "*"
"@storybook/addon-knobs@*":
peerDependencies:
react-dom: "*"
regenerator-runtime: "*"
"@storybook/addon-links@*":
peerDependencies:
react-dom: "*"
regenerator-runtime: "*"
"@storybook/addon-storysource@*":
peerDependencies:
react-dom: "*"
"@storybook/addons@*":
peerDependencies:
react-dom: "*"
regenerator-runtime: "*"
"@storybook/api@*":
peerDependencies:
react-dom: "*"
"@storybook/cli@*":
peerDependencies:
jest: "*"
"@storybook/client-api@*":
peerDependencies:
react-dom: "*"
regenerator-runtime: "*"
"@storybook/codemod@*":
peerDependencies:
jest: "*"
"@storybook/source-loader@*":
peerDependencies:
react-dom: "*"
"@storybook/vue@*":
peerDependencies:
"@babel/core": "*"
react: "*"
react-dom: "*"
"@vue/cli-plugin-typescript@*":
peerDependencies:
babel-loader: "*"
"@vue/cli-plugin-unit-jest@*":
peerDependencies:
vue: "*"
vue-template-compiler: "*"
"@vue/cli-service@*":
peerDependencies:
"@vue/cli-plugin-babel": "*"
"@vue/cli-plugin-eslint": "*"
"@vue/cli-plugin-typescript": "*"
"@vue/cli-plugin-unit-jest": "*"
vue-cli-plugin-quasar: "*"
eslint-plugin-vue@*:
peerDependencies:
vue-eslint-parser: "*"
react-color@*:
peerDependencies:
react: "*"
react-inspector@*:
peerDependencies:
"@storybook/core": "*"
rollup-plugin-vue@*:
peerDependencies:
postcss: "*"
storybook-router@*:
peerDependencies:
"@storybook/addons": "*"
"@storybook/core": "*"
react-dom: "*"
regenerator-runtime: "*"
vue-eslint-parser@*:
peerDependencies:
babel-eslint: "*"
"@firebase/database@*":
peerDependencies:
"@firebase/app-types": "*"
"@firebase/auth@*":
peerDependencies:
"@firebase/app-types": "*"
"@firebase/util": "*"
"@quasar/babel-preset-app@*":
peerDependencies:
"webpack": "*"
"@vue/eslint-config-standard@*":
peerDependencies:
"webpack": "*"
Is there any escape from this madness or is it a case of benching Berry until package maintainers make their packages PnP compatible? (Possibly delaying Berry uptake months/years)
@Robula You can actually help with PnP adoption, by adding these extensions into plugin-compat
, like it is done in this PR for example:
https://github.com/yarnpkg/berry/pull/805
If they will be added to plugin-compat
you won't have to list them in your projects over and over again. PRs on affected packages should be opened as well, however.
Is there any escape from this madness or is it a case of benching Berry until package maintainers make their packages PnP compatible? (Possibly delaying Berry uptake months/years)
Storybook is working on it and will soon be entirely compatible. For other definitions, we can add the most obvious fixes to plugin-compat as @larixer mentioned so that the efforts are mutualized between our users.
Helping us expand our E2E tests is also a very good way to prevent regressions in the projects we track (for example I know Vue CLI is compatible out of the box in the default workflow, but maybe the E2E test could be expanded to also test for TypeScript, since you seem to hit difficulties).
(Didn't mean to close, sorry, fat fingers)
In my case Storybook "just works" but I am on 6.0.0-alpha
.
@arcanis is there any chance to get ESLint support with PNP without need to install all sub dependencies of all plugins and configs? Or this question should be addressed to ESLint maintainers?
I think it's worth opening an issue on the ESLint repository - from my limited understanding, the problem is that the ESLint configuration doesn't resolve plugins relative to the configuration, so the fix would be on their side.
I was wondering the same as @kirill-konshin, I def. feel a bit stuck with my org-wise config. For now (horror!) I have resorted to using the node-modules plugin. I just cannot add all the eslint plugins to all repos and manage the upgrades on them.
There really seem to be no movement on the eslint side (from what I can see) and (from my reading in the old pnp eslint bugs) a general reluctance atm. So we seem to be between a rock and a hard place.
In the first comment a mention was made of some built-in react-create-app and gatsby override. Is there a way to get access to that for config in our projects? I suggesting so we can add our own roots.
... feeing like I check this issue 3 times a day ;)
@arcanis there is one: https://github.com/eslint/eslint/issues/3458, but maybe we should open another one.
@arcanis @not-an-aardvark I have figured out a not-very-good-looking but acceptable solution for my case: shareable config that is reused in the entire org.
Here's the outline:
main
entry point):const CLIEngine = require('eslint').CLIEngine;
const cli = new CLIEngine({
useEslintrc: false,
baseConfig: require('./config'),
resolvePluginsRelativeTo: __dirname,
});
module.exports = cli.getConfigForFile('dummy.ts');
const replaceInObject = (obj, key, newKey) => {
obj[newKey] = obj[key];
delete obj[key];
};
replaceInObject(
module.exports.settings['import/resolver'],
'node',
require.resolve('eslint-import-resolver-node'),
);
replaceInObject(
module.exports.settings['import/parsers'],
'@typescript-eslint/parser',
require.resolve('@typescript-eslint/parser'),
);
2 main ideas here: parse the config locally so that all extends
will become rules, all settings
will become true objects. Then resolve locally few things that were not resolved by plugins (I should probably do PRs or file issues).
bin
that use proper eslint
CLI with additional options:#!/usr/bin/env node
process.argv.push('--resolve-plugins-relative-to', __dirname);
require('eslint/bin/eslint');
.eslintrc.js
module.exports = {
extends: ['ringcentral-typescript']
};
{
"scripts": {
"lint": "rc-eslint"
}
}
Other option is to use ESLint as is but provide path explicitly. To do so the bin
should look like this:
#!/usr/bin/env node
process.stdout.write(__dirname);
And usage in project:
{
"scripts": {
"lint": "eslint --resolve-plugins-relative-to $(rc-eslint)"
}
}
The only problem so far is how to integrate custom bin with editors like WebStorm or VSCode. It would be 10 times easier if instead of a CLI/Node API option this parameter can be set in config...
Followed the approach by @kirill-konshin with just one tweak -
CLIEngine
magic there, just have parser: require.resolve('@typescript-eslint/parser')
in the shared config--resolve-plugins-relative-to ...
All seem ok and running. The editors are obviously problematic, in my case VSCode.
@jacogr I’m glad it was useful. In my case parser somehow was resolved by ESLint itself on step 2. And bin magic was introduced so that end users won’t have to deal with it so the config package stays self contained.
Regarding editors... it is really an issue because they can’t pick up raw config (without resolve plugins magic), so now I’m exploring options here.
Like I said before, it would be a magnitude easier if source directory for plugins can become part of config. As well as all my manual resolutions, which supposed to be done inside eslint...
... still testing through with actual use, so I still may need to resolve to your magic at some point, in one of the repos :)
I found out one drawback of solution with replacing ESLint binary:
// webpack config
const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin');
const PnpWebpackPlugin = require(`pnp-webpack-plugin`);
new ForkTsCheckerWebpackPlugin(
PnpWebpackPlugin.forkTsCheckerOptions({
checkSyntacticErrors: true,
eslint: true, // <---------- this requires eslint to be installed locally
memoryLimit: maxOldSpaceSize
})
}
So like I said before, CLI/API option does not give enough flexibility, it should be configured in ENV or straight in ESLint config (why not, if I resolve everything already, I might just add one more line to config).
Umpf, yes, That is an issue. May be an easy fix though since that plugin may be much easier to get a quick PR in.
(I am not that far up my tree yet, bumping my head with tsc and workspaces and pnp issues atm, so not at my webpack projects as of yet)
@jacogr @arcanis I've made another ugly workaround, maybe someone will find it helpful.
I've created a simple patch script in my shared config that brutally replaces resolvePluginsRelativeTo
in ESLint code. It works from console, works from WebStorm, and probably will work from VSCode too: https://github.com/ringcentral/ringcentral-javascript/blob/master/eslint-config-ringcentral-typescript/src/bin/rc-eslint-patch.js
#!/usr/bin/env node
/* eslint-disable @typescript-eslint/no-var-requires */
const fs = require('fs');
const patchPaths = [
require.resolve('eslint/lib/cli-engine/cascading-config-array-factory'),
require.resolve('eslint/lib/cli-engine/config-array-factory')
];
const pkg = require('../../package.json');
const token = 'resolvePluginsRelativeTo = cwd';
const addon = token.replace('cwd', `require.resolve('${pkg.name}') || cwd`);
patchPaths.forEach(path => fs.writeFileSync(path, fs.readFileSync(path).toString().replace(token, addon)));
Then in the app users need to add {"scripts": {"postinstall": "rc-eslint-patch"}}
.
And then unplug eslint
to allow script to make changes:
$ yarn unplug eslint
Looks like at the moment it's the only option since there are no other escape latches.
I have been running into a brick wall with VSCode & eslint with the shared config. Not quite excited about using postinstall scripts, but if it works... well, why not.
I am just juggling other balls atm, but will try and take a peek ASAP.
As always, thank you!
I added instruction to unplug eslint
, script won't work without it.
I am facing the same issue on a fresh CRA (3.4.1
) using berry
. It seems that the PR referenced in the first comment has been merged eslint/eslint#11388 (merged 6 days ago!). However, has not been released yet, not even in the last eslint v7-alpha. https://github.com/eslint/eslint/blob/master/CHANGELOG.md.
I have done something like this on my side: https://eshlox.net/2020/04/01/yarn-2-berry-typescript-vscode-prettier-eslint-fastify. It extracts the binaries for eslint into a file but plugins are still broken. I am going to try to add @kirill-konshin's logic to patch the custom imports. Thanks a lot for that script.
Hopefully that helps somewhat :)
It's possible to patch ESLint to have it require dependencies correctly by adding a require/import to your eslint config. The package @rushstack/eslint-patch fixes the way ESLint resolves dependencies (why don't they just fix this themselves :confused: ).
Add this to your eslint config and ESLint will start resolving things from where they are declared
import '@rushstack/eslint-patch/modern-module-resolution';
// or
require('@rushstack/eslint-patch/modern-module-resolution');
Fresh create-react-app 4.0.1, latest ESLint and using Yarn 2 with Zero-Installs/PNP. Same issue, and at a loss for how to get "eslintConfig": {"extends": ["react-app"]}
working on Yarn 2.
create-react-app
doesn't add eslint-config-react-app
as a dependency, until it's fixed you need to manually add it. yarn add eslint-config-react-app
It's fixed in https://github.com/facebook/create-react-app/pull/9872 but for some reason it wasn't included in 4.0.1
I have done something like this on my side: https://eshlox.net/2020/04/01/yarn-2-berry-typescript-vscode-prettier-eslint-fastify. It extracts the binaries for eslint into a file but plugins are still broken. I am going to try to add @kirill-konshin's logic to patch the custom imports. Thanks a lot for that script.
Hopefully that helps somewhat :)
Nice and simple guide but you haven't really initiated eslint
though. Did you mange to do that @kiily ?
Just commenting in case someone else runs into this in the future
eslint --resolve-plugins-relative-to $(yarn node -e \"console.log(require.resolve('eslint-plugin-foo/index.js'))\") .
I had a hard time getting the Eslint LSP server to work correctly in my Yarn PnP project that used workspaces. Using the CLI worked fine but LSP clients were not actually reporting any lint errors when they should have. I solved it as follows:
I extracted the ESlint config a separate package in the monorepo and referenced it as if it were a shared config in the workspace that I originally wanted to lint. I also applied the @rushstack/eslint-patch
for modern bundle resolution in the .eslintrc.cjs
file where I referenced the shared config, and I depended on the various ESlint plugins in the shared configuration.
After setting this up, the LSP clients reported lint errors correctly. I tested both vscode-eslint
and emacs-lsp
.
What package is covered by this investigations?
ESLint
Describe the goal of the investigation
I'd like ESLint to support PnP painlessly. It currently works, but requires a specific logic on our side, which is slightly suboptimal. This investigation aims to figure out how to improve this state.
Investigation report
Because of the PnP fallback mechanism, this problem doesn't occur in most cases (ie when the ESLint configuration file is your own). In this case even is ESLint doesn't list the plugin in its dependency PnP will still check which dependencies are listed at the top-level of your application.
This problem only happens when the ESLint configuration is part of a package you depend on. For example
react-scripts
, created bycreate-react-app
. In this instance, the plugins aren't listed in the application package.json, meaning that the fallback cannot kick in and do its job.In order to fix this, a temporary solution has been put into place:
react-scripts
andgatsby
(which are two of the most popular packages using this pattern) are used as secondary fallbacks if the top-level isn't enough. This is but a bandaid, since it causes other problems (other packages can forget to list their own dependencies ifreact-scripts
orgatsby
happen to depend on them), but should do the trick for now.The real fix is https://github.com/eslint/eslint/pull/11388, which is expected to land in ESLint 6.