Skip to content
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

Merged
merged 11 commits into from
Mar 22, 2024
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
]
},
"dependencies": {
"@typescript-eslint/utils": "^5.10.0"
"@typescript-eslint/utils": "^6.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

as well?

Copy link
Collaborator Author

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 now

Copy link
Member

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)

Copy link
Collaborator Author

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?

Copy link
Member

@SimenB SimenB Mar 21, 2024

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 😅 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure!

Copy link
Member

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!)

Copy link
Contributor

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.

Copy link
Collaborator Author

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

},
"devDependencies": {
"@babel/cli": "^7.4.4",
Expand Down
35 changes: 1 addition & 34 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { readdirSync } from 'fs';
import { join, parse } from 'path';
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import type { TSESLint } from '@typescript-eslint/utils';
import {
name as packageName,
version as packageVersion,
Expand All @@ -12,39 +12,6 @@ type RuleModule = TSESLint.RuleModule<string, unknown[]> & {
meta: Required<Pick<TSESLint.RuleMetaData<string>, 'docs'>>;
};

// v5 of `@typescript-eslint/experimental-utils` removed this
declare module '@typescript-eslint/utils/dist/ts-eslint/Rule' {
export interface RuleMetaDataDocs {
category: 'Best Practices' | 'Possible Errors';
}
}

declare module '@typescript-eslint/utils/dist/ts-eslint/SourceCode' {
export interface SourceCode {
/**
* Returns the scope of the given node.
* This information can be used track references to variables.
* @since 8.37.0
*/
getScope(node: TSESTree.Node): TSESLint.Scope.Scope;
/**
* Returns an array of the ancestors of the given node, starting at
* the root of the AST and continuing through the direct parent of the current node.
* This array does not include the currently-traversed node itself.
* @since 8.38.0
*/
getAncestors(node: TSESTree.Node): TSESTree.Node[];
/**
* Returns a list of variables declared by the given node.
* This information can be used to track references to variables.
* @since 8.38.0
*/
getDeclaredVariables(
node: TSESTree.Node,
): readonly TSESLint.Scope.Variable[];
}
}

// copied from https://github.com/babel/babel/blob/d8da63c929f2d28c401571e2a43166678c555bc4/packages/babel-helpers/src/helpers.js#L602-L606
/* istanbul ignore next */
const interopRequireDefault = (obj: any): { default: any } =>
Expand Down
104 changes: 92 additions & 12 deletions src/rules/__tests__/unbound-method.test.ts
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';

Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this always be true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also, we should use semver module for this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because we still support and test against @typescript-eslint/eslint-plugin v5

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll not replace this with semver since it'll just go away right after this PR when we drop support for v5

? ['const x = Object.defineProperty;']
: []),
...[
'instance.bound();',
'instance.unbound();',
Expand Down Expand Up @@ -455,8 +481,8 @@ class OtherClass extends BaseClass {
const oc = new OtherClass();
oc.superLogThis();
`,
],
invalid: [
]),
invalid: withFixtureFilename([
{
code: `
class Console {
Expand Down Expand Up @@ -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',
},
],
},
]),
});
4 changes: 2 additions & 2 deletions src/rules/consistent-test-it.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export default createRule<
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Enforce `test` and `it` usage conventions',
recommended: false,
},
fixable: 'code',
messages: {
Expand All @@ -51,9 +49,11 @@ export default createRule<
type: 'object',
properties: {
fn: {
type: 'string',
enum: [TestCaseName.it, TestCaseName.test],
},
withinDescribe: {
type: 'string',
enum: [TestCaseName.it, TestCaseName.test],
},
},
Expand Down
2 changes: 0 additions & 2 deletions src/rules/expect-expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ export default createRule<
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Enforce assertion to be made in a test body',
recommended: 'warn',
},
messages: {
noAssertions: 'Test has no assertions',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/max-expects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Enforces a maximum number assertion calls in a test body',
recommended: false,
},
messages: {
exceededMaxAssertion:
Expand Down
2 changes: 0 additions & 2 deletions src/rules/max-nested-describe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Enforces a maximum depth to nested describe calls',
recommended: false,
},
messages: {
exceededMaxDepth:
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-alias-methods.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow alias methods',
recommended: 'error',
},
messages: {
replaceAlias: `Replace {{ alias }}() with its canonical name of {{ canonical }}()`,
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-commented-out-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow commented out tests',
recommended: 'warn',
},
messages: {
commentedTests: 'Some tests seem to be commented',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-conditional-expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ export default createRule({
meta: {
docs: {
description: 'Disallow calling `expect` conditionally',
category: 'Best Practices',
recommended: 'error',
},
messages: {
conditionalExpect: 'Avoid calling `expect` conditionally`',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-conditional-in-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ export default createRule({
meta: {
docs: {
description: 'Disallow conditional logic in tests',
category: 'Best Practices',
recommended: false,
},
messages: {
conditionalInTest: 'Avoid having conditionals in tests',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-confusing-set-timeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow confusing usages of jest.setTimeout',
recommended: false,
},
messages: {
globalSetTimeout: '`jest.setTimeout` should be call in `global` scope',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-deprecated-functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow use of deprecated functions',
recommended: 'error',
},
messages: {
deprecatedFunction:
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-disabled-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow disabled tests',
recommended: 'warn',
},
messages: {
missingFunction: 'Test is missing function argument',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-done-callback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow using a callback in asynchronous tests and hooks',
recommended: 'error',
},
messages: {
noDoneCallback:
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-duplicate-hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow duplicate setup and teardown hooks',
recommended: false,
},
messages: {
noDuplicateHook: 'Duplicate {{hook}} in describe block',
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow using `exports` in files containing tests',
recommended: 'error',
},
messages: {
unexpectedExport: `Do not export from a test file`,
Expand Down
2 changes: 0 additions & 2 deletions src/rules/no-focused-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ export default createRule({
name: __filename,
meta: {
docs: {
category: 'Best Practices',
description: 'Disallow focused tests',
recommended: 'error',
},
messages: {
focusedTest: 'Unexpected focused test',
Expand Down
Loading
Loading