Skip to content

refactor(eslint-plugin-react-hooks): change array type and improve conditionals #32400

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 17, 2025
Merged

refactor(eslint-plugin-react-hooks): change array type and improve conditionals #32400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7df534
build(eslint-plugin-react-hooks): add ts-linting
michaelfaith Feb 16, 2025
077a4b0
refactor(eslint-plugin-react-hooks): improve conditionals
michaelfaith Feb 16, 2025
6ea4c0d
ci (eslint-e2e): exclude nested `node_modules` from cache
michaelfaith Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,10 +446,7 @@ module.exports = {
},
},
{
files: [
'scripts/eslint-rules/*.js',
'packages/eslint-plugin-react-hooks/src/*.js',
],
files: ['scripts/eslint-rules/*.js'],
plugins: ['eslint-plugin'],
rules: {
'eslint-plugin/prefer-object-rule': ERROR,
Expand Down Expand Up @@ -517,6 +514,26 @@ module.exports = {
__IS_INTERNAL_VERSION__: 'readonly',
},
},
{
files: ['packages/eslint-plugin-react-hooks/src/**/*'],
extends: ['plugin:@typescript-eslint/recommended'],
parser: '@typescript-eslint/parser',
plugins: ['@typescript-eslint', 'eslint-plugin'],
rules: {
'@typescript-eslint/no-explicit-any': OFF,
'@typescript-eslint/no-non-null-assertion': OFF,
'@typescript-eslint/array-type': [ERROR, {default: 'generic'}],

'es/no-optional-chaining': OFF,

'eslint-plugin/prefer-object-rule': ERROR,
'eslint-plugin/require-meta-fixable': [
ERROR,
{catchNoFixerButFixableProperty: true},
],
'eslint-plugin/require-meta-has-suggestions': ERROR,
},
},
],

env: {
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/runtime_eslint_plugin_e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:
uses: actions/cache@v4
id: node_modules
with:
path: "**/node_modules"
path: "node_modules"
Copy link
Member

Choose a reason for hiding this comment

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

Was this just to bust the cache? I don't see why what it was previously needs to be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The node_modules within the fixtures are soft-linking to the build directory for the eslint-plugin. When those node_modules were fulfilled from cache, the symlinks were broken, and after the build, it's not re-linked. So this changes it so that it's still caching the root node_modules, but not the nested ones. That way those node_modules with the symlinks can be created each time.

key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }}
- name: Ensure clean build directory
run: rm -rf build
Expand Down
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
"@rollup/plugin-node-resolve": "^15.0.1",
"@rollup/plugin-replace": "^5.0.2",
"@rollup/plugin-typescript": "^12.1.2",
"@typescript-eslint/eslint-plugin": "^6.21.0",
"@typescript-eslint/parser": "^6.21.0",
"abortcontroller-polyfill": "^1.7.5",
"art": "0.10.1",
"babel-plugin-syntax-trailing-function-commas": "^6.5.0",
Expand Down
32 changes: 17 additions & 15 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type DeclaredDependency = {

type Dependency = {
isStable: boolean;
references: Scope.Reference[];
references: Array<Scope.Reference>;
};

type DependencyTreeNode = {
Expand Down Expand Up @@ -184,7 +184,9 @@ const rule = {
// Get the current scope.
const scope = scopeManager.acquire(node);
if (!scope) {
return;
throw new Error(
'Unable to acquire scope for the current node. This is a bug in eslint-plugin-react-hooks, please file an issue.',
);
}

// Find all our "pure scopes". On every re-render of a component these
Expand Down Expand Up @@ -255,7 +257,7 @@ const rule = {
// Detect primitive constants
// const foo = 42
let declaration = defNode.parent;
if (declaration == null && componentScope) {
if (declaration == null && componentScope != null) {
// This might happen if variable is declared after the callback.
// In that case ESLint won't set up .parent refs.
// So we'll set them up manually.
Expand All @@ -266,7 +268,7 @@ const rule = {
}
}
if (
declaration &&
declaration != null &&
'kind' in declaration &&
declaration.kind === 'const' &&
init.type === 'Literal' &&
Expand Down Expand Up @@ -454,7 +456,7 @@ const rule = {
function isInsideEffectCleanup(reference: Scope.Reference): boolean {
let curScope: Scope.Scope | null = reference.from;
let isInReturnedFunction = false;
while (curScope && curScope.block !== node) {
while (curScope != null && curScope.block !== node) {
if (curScope.type === 'function') {
isInReturnedFunction =
curScope.block.parent != null &&
Expand Down Expand Up @@ -529,7 +531,7 @@ const rule = {
continue;
}
// Ignore references to the function itself as it's not defined yet.
if (def.node && def.node.init === node.parent) {
if (def.node != null && def.node.init === node.parent) {
continue;
}
// Ignore Flow type parameters
Expand Down Expand Up @@ -566,8 +568,8 @@ const rule = {
// Is React managing this ref or us?
// Let's see if we can find a .current assignment.
let foundCurrentAssignment = false;
for (const reference of references) {
const {identifier} = reference;
for (const ref of references) {
const {identifier} = ref;
const {parent} = identifier;
if (
parent != null &&
Expand Down Expand Up @@ -660,7 +662,7 @@ const rule = {
}

let fnScope: Scope.Scope | null = reference.from;
while (fnScope && fnScope.type !== 'function') {
while (fnScope != null && fnScope.type !== 'function') {
fnScope = fnScope.upper;
}
const isDirectlyInsideEffect = fnScope?.block === node;
Expand Down Expand Up @@ -704,7 +706,7 @@ const rule = {
return;
}

const declaredDependencies: DeclaredDependency[] = [];
const declaredDependencies: Array<DeclaredDependency> = [];
const externalDependencies = new Set<string>();
const isArrayExpression =
declaredDependenciesNode.type === 'ArrayExpression';
Expand Down Expand Up @@ -1469,7 +1471,7 @@ function collectRecommendations({
isEffect,
}: {
dependencies: Map<string, Dependency>;
declaredDependencies: DeclaredDependency[];
declaredDependencies: Array<DeclaredDependency>;
stableDependencies: Set<string>;
externalDependencies: Set<string>;
isEffect: boolean;
Expand Down Expand Up @@ -1592,7 +1594,7 @@ function collectRecommendations({
}

// Collect suggestions in the order they were originally specified.
const suggestedDependencies: string[] = [];
const suggestedDependencies: Array<string> = [];
const unnecessaryDependencies = new Set<string>();
const duplicateDependencies = new Set<string>();
declaredDependencies.forEach(({key}) => {
Expand Down Expand Up @@ -1699,7 +1701,7 @@ function scanForConstructions({
componentScope,
scope,
}: {
declaredDependencies: DeclaredDependency[];
declaredDependencies: Array<DeclaredDependency>;
declaredDependenciesNode: Node;
componentScope: Scope.Scope;
scope: Scope.Scope;
Expand Down Expand Up @@ -1747,7 +1749,7 @@ function scanForConstructions({
}
return null;
})
.filter(Boolean) as [Scope.Variable, string][];
.filter(Boolean) as Array<[Scope.Variable, string]>;

function isUsedOutsideOfHook(ref: Scope.Variable): boolean {
let foundWriteExpr = false;
Expand Down Expand Up @@ -2007,7 +2009,7 @@ function fastFindReferenceWithParent(start: Node, target: Node): Node | null {
return null;
}

function joinEnglish(arr: string[]): string {
function joinEnglish(arr: Array<string>): string {
let s = '';
for (let i = 0; i < arr.length; i++) {
s += arr[i];
Expand Down
12 changes: 7 additions & 5 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,10 @@ const rule = {
},
create(context: Rule.RuleContext) {
let lastEffect: CallExpression | null = null;
const codePathReactHooksMapStack: Map<Rule.CodePathSegment, Node[]>[] = [];
const codePathSegmentStack: Rule.CodePathSegment[] = [];
const codePathReactHooksMapStack: Array<
Map<Rule.CodePathSegment, Array<Node>>
> = [];
const codePathSegmentStack: Array<Rule.CodePathSegment> = [];
const useEffectEventFunctions = new WeakSet();

// For a given scope, iterate through the references and add all useEffectEvent definitions. We can
Expand Down Expand Up @@ -190,7 +192,7 @@ const rule = {
// Maintain code path stack as we traverse.
onCodePathStart: () =>
codePathReactHooksMapStack.push(
new Map<Rule.CodePathSegment, Node[]>(),
new Map<Rule.CodePathSegment, Array<Node>>(),
),

// Process our code path.
Expand Down Expand Up @@ -561,7 +563,7 @@ const rule = {
context.report({node: hook, message});
}
} else if (
codePathNode.parent &&
codePathNode.parent != null &&
(codePathNode.parent.type === 'MethodDefinition' ||
// @ts-expect-error `ClassProperty` was removed from typescript-estree in https://github.com/typescript-eslint/typescript-eslint/pull/3806
codePathNode.parent.type === 'ClassProperty' ||
Expand Down Expand Up @@ -758,7 +760,7 @@ function getFunctionName(node: Node) {
/**
* Convenience function for peeking the last item in a stack.
*/
function last<T>(array: T[]): T {
function last<T>(array: Array<T>): T {
return array[array.length - 1] as T;
}

Expand Down
50 changes: 49 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,11 @@
dependencies:
eslint-visitor-keys "^3.3.0"

"@eslint-community/regexpp@^4.5.1":
version "4.12.1"
resolved "https://registry.yarnpkg.com/@eslint-community/regexpp/-/regexpp-4.12.1.tgz#cfc6cffe39df390a3841cde2abccf92eaa7ae0e0"
integrity sha512-CCZCDJuduB9OUkFkY2IgppNZMi2lBQgD2qzwXkEia16cge2pijY/aXi96CJMquDMn3nJdlPV1A5KrJEXwfLNzQ==

"@eslint-community/regexpp@^4.6.1":
version "4.10.0"
resolved "https://registry.yarnpkg.com/@eslint-community/regexpp/-/regexpp-4.10.0.tgz#548f6de556857c8bb73bbee70c35dc82a2e74d63"
Expand Down Expand Up @@ -3718,6 +3723,23 @@
dependencies:
"@types/node" "*"

"@typescript-eslint/eslint-plugin@^6.21.0":
version "6.21.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-6.21.0.tgz#30830c1ca81fd5f3c2714e524c4303e0194f9cd3"
integrity sha512-oy9+hTPCUFpngkEZUSzbf9MxI65wbKFoQYsgPdILTfbUldp5ovUuphZVe4i30emU9M/kP+T64Di0mxl7dSw3MA==
dependencies:
"@eslint-community/regexpp" "^4.5.1"
"@typescript-eslint/scope-manager" "6.21.0"
"@typescript-eslint/type-utils" "6.21.0"
"@typescript-eslint/utils" "6.21.0"
"@typescript-eslint/visitor-keys" "6.21.0"
debug "^4.3.4"
graphemer "^1.4.0"
ignore "^5.2.4"
natural-compare "^1.4.0"
semver "^7.5.4"
ts-api-utils "^1.0.1"

"@typescript-eslint/[email protected]":
version "2.34.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/experimental-utils/-/experimental-utils-2.34.0.tgz#d3524b644cdb40eebceca67f8cf3e4cc9c8f980f"
Expand Down Expand Up @@ -3780,6 +3802,17 @@
"@typescript-eslint/typescript-estree" "5.62.0"
debug "^4.3.4"

"@typescript-eslint/parser@^6.21.0":
version "6.21.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/parser/-/parser-6.21.0.tgz#af8fcf66feee2edc86bc5d1cf45e33b0630bf35b"
integrity sha512-tbsV1jPne5CkFQCgPBcDOt30ItF7aJoZL997JSF7MhGQqOeT3svWRYxiqlfA5RUdlHN6Fi+EI9bxqbdyAUZjYQ==
dependencies:
"@typescript-eslint/scope-manager" "6.21.0"
"@typescript-eslint/types" "6.21.0"
"@typescript-eslint/typescript-estree" "6.21.0"
"@typescript-eslint/visitor-keys" "6.21.0"
debug "^4.3.4"

"@typescript-eslint/[email protected]":
version "4.1.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/scope-manager/-/scope-manager-4.1.0.tgz#9e389745ee9cfe12252ed1e9958808abd6b3a683"
Expand All @@ -3804,6 +3837,16 @@
"@typescript-eslint/types" "6.21.0"
"@typescript-eslint/visitor-keys" "6.21.0"

"@typescript-eslint/[email protected]":
version "6.21.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/type-utils/-/type-utils-6.21.0.tgz#6473281cfed4dacabe8004e8521cee0bd9d4c01e"
integrity sha512-rZQI7wHfao8qMX3Rd3xqeYSMCL3SoiSQLBATSiVKARdFGCYSRvmViieZjqc58jKgs8Y8i9YvVVhRbHSTA4VBag==
dependencies:
"@typescript-eslint/typescript-estree" "6.21.0"
"@typescript-eslint/utils" "6.21.0"
debug "^4.3.4"
ts-api-utils "^1.0.1"

"@typescript-eslint/[email protected]":
version "3.10.1"
resolved "https://registry.yarnpkg.com/@typescript-eslint/types/-/types-3.10.1.tgz#1d7463fa7c32d8a23ab508a803ca2fe26e758727"
Expand Down Expand Up @@ -3892,7 +3935,7 @@
semver "^7.5.4"
ts-api-utils "^1.0.1"

"@typescript-eslint/utils@^6.0.0":
"@typescript-eslint/utils@6.21.0", "@typescript-eslint/utils@^6.0.0":
version "6.21.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/utils/-/utils-6.21.0.tgz#4714e7a6b39e773c1c8e97ec587f520840cd8134"
integrity sha512-NfWVaC8HP9T8cbKQxHcsJBY5YE1O33+jpMwN45qzWWaPDZgLIbo12toGMWnmhvCpd3sIxkpDw3Wv1B3dYrbDQQ==
Expand Down Expand Up @@ -9799,6 +9842,11 @@ ignore@^5.2.0:
resolved "https://registry.yarnpkg.com/ignore/-/ignore-5.3.1.tgz#5073e554cd42c5b33b394375f538b8593e34d4ef"
integrity sha512-5Fytz/IraMjqpwfd34ke28PTVMjZjJG2MPn5t7OE4eUCUNf8BAa7b5WUS9/Qvr6mwOQS7Mk6vdsMno5he+T8Xw==

ignore@^5.2.4:
version "5.3.2"
resolved "https://registry.yarnpkg.com/ignore/-/ignore-5.3.2.tgz#3cd40e729f3643fd87cb04e50bf0eb722bc596f5"
integrity sha512-hsBTNUqQTDwkWtcdYI2i06Y/nUBEsNEDJKjWdigLvegy8kDuJAS8uRlpkkcQpyEXL0Z/pjDy5HBmMjRCJ2gq+g==

[email protected]:
version "1.1.1"
resolved "https://registry.yarnpkg.com/image-size/-/image-size-1.1.1.tgz#ddd67d4dc340e52ac29ce5f546a09f4e29e840ac"
Expand Down
Loading