-
-
Notifications
You must be signed in to change notification settings - Fork 593
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(eslint): supports eslint v9 #1729
Conversation
a92a5a5
to
92d6003
Compare
packages/eslint/test/test.mjs
Outdated
count += results[0].messages.length; | ||
// eslint-disable-next-line prefer-destructuring | ||
const { message } = results[0].messages[0]; | ||
t.is(message, "'x' is not defined."); |
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.
we don't modify existing tests - that presents a surface area for regressions. instead, we create new tests with the new conditions we'd like to test. I doubt that ESLint would have introduced a breaking change in an 8.x minor version, so this means that the conditions of the test changed. [unless I'm mistaken in some way] please revert this one and create a new test for the new messages and conditions you'd like to test.
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.
sure, I'll change these later
packages/eslint/test/test.mjs
Outdated
{ message: /Found 1 error/ } | ||
); | ||
}); | ||
// test('should fail with enabled throwOnError option', async (t) => { |
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.
why was this commented out?
packages/eslint/test/test.mjs
Outdated
fs.readFileSync('./test/fixtures/fixable-clone.js').toString(), | ||
fs.readFileSync('./test/fixtures/fixed.js').toString() | ||
fs.readFileSync('fixable-clone.js').toString(), | ||
`\r\n\r\nfunction foo() {\r\n return true;\r\n}\r\n\r\nfoo();` |
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.
here again, new tests are preferred to modifying existing tests' expectations
packages/eslint/test/test.mjs
Outdated
// eslint-disable-next-line prefer-destructuring | ||
const { message } = results[0].messages[0]; | ||
t.is(message, "'x' is not defined."); | ||
const { messages } = results[0]; |
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.
here again, new tests are preferred to modifying existing tests' expectations
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.
please see the comments I left. there are some clarifications that are needed before we can move this forward
I'm sorry, but I'm unable to move this forward. If anyone is interested, feel free to make a new PR. |
Rollup Plugin Name:
@rollup/plugin-eslint
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: resolves #1708
Description
This PR makes
@rollup/plugin-eslint
support eslint v9 by simply bumping the eslint version. Ava tests are adjusted to match its needs. Let me know if I miss something.