-
Notifications
You must be signed in to change notification settings - Fork 242
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: upgrade @typescript-eslint/utils
to v6
#1508
Changes from all commits
6d56a7d
7bccb14
4a0256c
8980cb5
d40f3ca
191e892
e9d2f1e
05171a2
b709f7e
b8d120d
e1024e7
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,6 @@ | ||
import path from 'path'; | ||
import { ESLintUtils, type TSESLint } from '@typescript-eslint/utils'; | ||
import { version as rawTypeScriptESLintPluginVersion } from '@typescript-eslint/eslint-plugin/package.json'; | ||
import { TSESLint } from '@typescript-eslint/utils'; | ||
import dedent from 'dedent'; | ||
import type { MessageIds, Options } from '../unbound-method'; | ||
|
||
|
@@ -9,15 +10,35 @@ function getFixturesRootDir(): string { | |
|
||
const rootPath = getFixturesRootDir(); | ||
|
||
const ruleTester = new ESLintUtils.RuleTester({ | ||
parser: '@typescript-eslint/parser', | ||
const ruleTester = new TSESLint.RuleTester({ | ||
parser: require.resolve('@typescript-eslint/parser'), | ||
parserOptions: { | ||
sourceType: 'module', | ||
tsconfigRootDir: rootPath, | ||
project: './tsconfig.json', | ||
}, | ||
}); | ||
|
||
const fixtureFilename = path.join(rootPath, 'file.ts'); | ||
|
||
const withFixtureFilename = < | ||
T extends Array< | ||
| (TSESLint.ValidTestCase<Options> | string) | ||
| TSESLint.InvalidTestCase<MessageIds, Options> | ||
>, | ||
>( | ||
cases: T, | ||
): T extends Array<TSESLint.InvalidTestCase<MessageIds, Options>> | ||
? Array<TSESLint.InvalidTestCase<MessageIds, Options>> | ||
: Array<TSESLint.ValidTestCase<Options>> => { | ||
// @ts-expect-error this is fine, and will go away later once we upgrade | ||
return cases.map(code => { | ||
const test = typeof code === 'string' ? { code } : code; | ||
|
||
return { filename: fixtureFilename, ...test }; | ||
}); | ||
}; | ||
|
||
const ConsoleClassAndVariableCode = dedent` | ||
class Console { | ||
log(str) { | ||
|
@@ -164,8 +185,8 @@ describe('error handling', () => { | |
}); | ||
|
||
describe('when @typescript-eslint/eslint-plugin is not available', () => { | ||
const ruleTester = new ESLintUtils.RuleTester({ | ||
parser: '@typescript-eslint/parser', | ||
const ruleTester = new TSESLint.RuleTester({ | ||
parser: require.resolve('@typescript-eslint/parser'), | ||
parserOptions: { | ||
sourceType: 'module', | ||
tsconfigRootDir: rootPath, | ||
|
@@ -177,16 +198,18 @@ describe('error handling', () => { | |
'unbound-method jest edition without type service', | ||
requireRule(true), | ||
{ | ||
valid: validTestCases.concat(invalidTestCases.map(({ code }) => code)), | ||
valid: withFixtureFilename( | ||
validTestCases.concat(invalidTestCases.map(({ code }) => code)), | ||
), | ||
invalid: [], | ||
}, | ||
); | ||
}); | ||
}); | ||
|
||
ruleTester.run('unbound-method jest edition', requireRule(false), { | ||
valid: validTestCases, | ||
invalid: invalidTestCases, | ||
valid: withFixtureFilename(validTestCases), | ||
invalid: withFixtureFilename(invalidTestCases), | ||
}); | ||
|
||
function addContainsMethodsClass(code: string): string { | ||
|
@@ -225,11 +248,14 @@ function addContainsMethodsClassInvalid( | |
} | ||
|
||
ruleTester.run('unbound-method', requireRule(false), { | ||
valid: [ | ||
valid: withFixtureFilename([ | ||
'Promise.resolve().then(console.log);', | ||
"['1', '2', '3'].map(Number.parseInt);", | ||
'[5.2, 7.1, 3.6].map(Math.floor);', | ||
'const x = console.log;', | ||
...(parseInt(rawTypeScriptESLintPluginVersion.split('.')[0], 10) >= 6 | ||
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. won't this always be true? 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. (also, we should use 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. No because we still support and test against 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 not replace this with |
||
? ['const x = Object.defineProperty;'] | ||
: []), | ||
...[ | ||
'instance.bound();', | ||
'instance.unbound();', | ||
|
@@ -455,8 +481,8 @@ class OtherClass extends BaseClass { | |
const oc = new OtherClass(); | ||
oc.superLogThis(); | ||
`, | ||
], | ||
invalid: [ | ||
]), | ||
invalid: withFixtureFilename([ | ||
{ | ||
code: ` | ||
class Console { | ||
|
@@ -762,5 +788,59 @@ class OtherClass extends BaseClass { | |
}, | ||
], | ||
}, | ||
], | ||
{ | ||
code: ` | ||
const values = { | ||
a() {}, | ||
b: () => {}, | ||
}; | ||
|
||
const { a, b } = values; | ||
`, | ||
errors: [ | ||
{ | ||
line: 7, | ||
column: 9, | ||
endColumn: 10, | ||
messageId: 'unboundWithoutThisAnnotation', | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const values = { | ||
a() {}, | ||
b: () => {}, | ||
}; | ||
|
||
const { a: c } = values; | ||
`, | ||
errors: [ | ||
{ | ||
line: 7, | ||
column: 9, | ||
endColumn: 10, | ||
messageId: 'unboundWithoutThisAnnotation', | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const values = { | ||
a() {}, | ||
b: () => {}, | ||
}; | ||
|
||
const { b, a } = values; | ||
`, | ||
errors: [ | ||
{ | ||
line: 7, | ||
column: 12, | ||
endColumn: 13, | ||
messageId: 'unboundWithoutThisAnnotation', | ||
}, | ||
], | ||
}, | ||
]), | ||
}); |
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.
as well?
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're still supporting
@typescript-eslint/eslint-plugin
v5 and don't use a lot of the actual rules so I don't think there's a strong advantage - I'll double check that but I don't think it should block landing this right nowThere 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.
I think we can drop it - no reason too keep support when bumping version (when we're landing as major regardless)
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.
ok but can we do that in a PR after this so it gets a dedicated changelog entry?
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.
i.e. I think it makes sense to add support for further majors as a minor, but if we're doing a major release regardless, I don't think it's super valuable (beyond this special case when we're 2 majors behind 😅 )
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.
for sure!
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.
(sorry, I had the tab open and GH isn't super good with live updates 😅 let's blame rails!)
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.
Suggestion: update this to the last v6 version.
That way you're enforcing that package managers always give you the latest version, rather than trying to resolve to some random one with fewer bug fixes.
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.
I tend to favor not doing that to enable better deduplication downstream, given we consider this a very stable dependency