-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ESLint #8
Comments
👋 I had a few questions what Plug n' Play will look like with ESLint after eslint/eslint#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 eslint/eslint#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
|
👋 !
If the editors invoke ESLint through its CLI interface they'll just have to invoke If the editors do more exotic things like running an internal instance of ESLint that
Based on what I understand from eslint/eslint#11388 it seems like |
Thanks, that makes sense. When using something like
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 Thanks, I think that answers my questions -- I might propose some separate changes to ESLint based on this info. |
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:
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 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 @arcanis - for context, I've got a project that contains a workspace named |
I'm not sure I understand the question, but it is configurable using the |
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.
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 I haven't found a way to rewrite something like I also tried to explicitly list plugins in |
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 |
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 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 |
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 |
@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: eslint/eslint#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: Step 1. Create config as usualStep 2. Create a wrapper on top of it (which will be
|
Followed the approach by @kirill-konshin with just one tweak -
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:
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 #!/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 And then unplug $ 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 |
I am facing the same issue on a fresh CRA ( 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 :) |
* [eslint-config] add eslint-config package * Bump-up pnpify * [eslint-plugin] rename eslint-config to eslint-plugin * Setup ESLint * [eslint-plugin] use js instead of json to avoid yarn 2 resolution issue * [eslint-plugin] disabled @typescript-eslint's indent rule * bump eslint pnpify version * Fix format * [eslint-plugin] ignore max-len for comments * [eslint-plugin] add default option for typescript projects * [eslint-plugin] set quote-props rule as consistent-as-needed * enable eslint for workspace by default * cleanup gitattributes * [eslint-plugin] release minor ---- Issues: - Need to install plugins even eslint-plugin package provide it as dependencies. (yarnpkg/berry#8) - Cannot check indentations of TypeScript files properly. (typescript-eslint/typescript-eslint#1824)
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 😕 ). Add this to your eslint config and ESLint will start resolving things from where they are declared
|
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 |
|
…(comment) * upgrade yarn to 3.0.1 and also for other packages + add missing deps: simplebar and libarchive.js
…(comment) * upgrade yarn to 3.0.1 and also for other packages + add missing deps: simplebar & libarchive.js & @types/wicg-file-system-access
Nice and simple guide but you haven't really initiated |
Just commenting in case someone else runs into this in the future |
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 After setting this up, the LSP clients reported lint errors correctly. I tested both |
It seems that with the introduction of ESLint flat config, this issue is no longer relevant |
Only once most ecosystem actually adopts it |
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 eslint/eslint#11388, which is expected to land in ESLint 6.
The text was updated successfully, but these errors were encountered: