-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat: use ESLint
instead of CLIEngine
when available
#128
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
const { ESLint } = require('eslint'); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems pretty valuable to try to support both eslint 8 and < 8 at the same time There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the error message from eslint is pretty good 👍 Specifically in this case if I leave it as an array
|
||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure - the (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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}, | ||
"devDependencies": { | ||
"@babel/cli": "^7.10.4", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed for optional chaining (babel transpiles it away)