feat: use ESLint instead of CLIEngine when available
#128
Conversation
SimenB
force-pushed
the
avoid-engine
branch
from
3009a5f to
86226f5
Compare
| module.exports = { | ||
| cliOptions: { | ||
| global: ['hello'], | ||
| // `ESLint` requires this to be an object, not an array | ||
| global: ESLint ? { hello: true } : ['hello'], |
There was a problem hiding this comment.
this is main reason I think this is a breaking change. While it's easy enough to normalize this specific field, there's probably others we'd miss - cleaner to just make a breaking change
There was a problem hiding this comment.
thinking about it, #125 might be considered breaking as well, if people previously ignored an engine warning and used this package on node 6. I don't consider it a breaking change, but bundling these together in a new major removes any ambiguity 🤷
There was a problem hiding this comment.
It seems pretty valuable to try to support both eslint 8 and < 8 at the same time
There was a problem hiding this comment.
sure, but I don't think it's our responsibility to coerce user input into the correct type. Consumers should configure the the version of ESLint they choose to use correctly.
(I know we do some coercion already, so I'm also fine with special casing globals so our tests are happy. I'd still consider this a breaking change since it's a moving target though)
There was a problem hiding this comment.
Oh true, fair point - i think it’s 100% fine if a user switches to eslint 8, and this runner breaks until they also fix their configuration (altho hopefully they get a very clear error message)
There was a problem hiding this comment.
Yeah, the error message from eslint is pretty good 👍 Specifically in this case if I leave it as an array
ESLint configuration in CLIOptions is invalid:
- Property "globals" is the wrong type (expected object but got `["hello"]`).
| @@ -1,6 +1,9 @@ | |||
| { | |||
| "extends": ["airbnb-base", "prettier"], | |||
| "plugins": ["prettier", "jest"], | |||
| "parserOptions": { | |||
| "ecmaVersion": 11 | |||
There was a problem hiding this comment.
needed for optional chaining (babel transpiles it away)
BREAKING CHANGE: Using `ESLint` might need different configuration options. See https://eslint.org/docs/developer-guide/nodejs-api#parameters
SimenB
force-pushed
the
avoid-engine
branch
from
86226f5 to
f6c72e1
Compare
| @@ -26,7 +26,8 @@ | |||
| "dependencies": { | |||
| "chalk": "^3.0.0", | |||
| "cosmiconfig": "^6.0.0", | |||
| "create-jest-runner": "^0.6.0" | |||
| "create-jest-runner": "^0.6.0", | |||
| "dot-prop": "^5.3.0" | |||
There was a problem hiding this comment.
This author is planning to make all their packages ESM-only - I’d strongly suggest avoiding adding them as a dependency.
There was a problem hiding this comment.
I went for v5 since v6 doesn't support node 8. We can migrate to some other module later if needed. Unless you have a good suggestion for a module that's not this or lodash? 😀
There was a problem hiding this comment.
Do we need a module for this? It always seems hacky to me to rely on converting a dotted string to object navigation; maybe there’s a different approach?
There was a problem hiding this comment.
not sure - the ESLint class requires a bunch of stuff in configOverrides instead of in the root object, and deep merging is quite hellish in JS (without some module). Going with a dotted setter seemed like a nice shortcut for the specific CLI options we've added support for
(I'm not entirely sure why we have this normalization instead of passing through raw as-is, but that's a separate discussion)
There was a problem hiding this comment.
I'll land this, and if we figure out some cleaner way of dealing with this we can revisit. It's not exposed, so it's an implementation detail we can change at will
BREAKING CHANGE: Using
ESLintmight need different configurationoptions. See https://eslint.org/docs/developer-guide/nodejs-api#parameters
CLIEngineis deprecated in ESLint v7 and removed entirely in ESLint v8, replaced byESLint.Landing this might give us support for ESLint v8 (#123) without changing anything more (although peer dep might still warn)