fix: no-unnecessary-waiting flag identifiers defined in object/array patterns#308
Open
overlookmotel wants to merge 1 commit intocypress-io:masterfrom
Open
fix: no-unnecessary-waiting flag identifiers defined in object/array patterns#308overlookmotel wants to merge 1 commit intocypress-io:masterfrom
no-unnecessary-waiting flag identifiers defined in object/array patterns#308overlookmotel wants to merge 1 commit intocypress-io:masterfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR fixes no-unnecessary-waiting scope-definition handling to work with TS-ESLint/Oxlint scope managers (which don’t provide deprecated definition.index) and to avoid crashes on non-parameter definition types, while also expanding detection to destructured parameters with numeric defaults.
Changes:
- Replace parameter default detection logic from
definition.node.params[definition.index]to usingdefinition.name.parentto avoid deprecated/missingindex. - Add test coverage for destructured parameters (with and without numeric defaults) and for “weird” non-parameter definition types (function/class/catch).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/rules/no-unnecessary-waiting.js | Updates scope-definition traversal to use definition.name.parent for default-value detection and avoid TypeError on non-parameter defs. |
| tests/lib/rules/no-unnecessary-waiting.js | Adds valid/invalid cases covering destructured defaults and non-parameter definition types to prevent regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix 2 related but separate problems with
no-unnecessary-waitingrule.Problem 1: Incompatibility with TS-ESLint and Oxlint
I came across this problem when a contributor added
eslint-plugin-cypressto Oxlint's conformance testing. oxc-project/oxc#20265. We noticed that 3 tests for this rule were failing.no-unnecessary-waitingrule usesdefinition.indexto look up parameter nodes when checking if a function parameter has a numeric default value. This property is part ofeslint-scope'sDefinitionclass, but it's listed as deprecated in ESLint's Scope Manager Interface docs. For this reason, TS-ESLint's scope manager doesn't implement theindexproperty it at all. Oxlint (if you care about us) follows TS-ESLint, and therefore doesn't implement it either.This means that if user is using TS-ESLint or Oxlint, the rule silently fails to detect cases like:
In TS-ESLint or Oxlint,
definition.indexisundefined, sodefinition.node.params[undefined]returnsundefined, and the rule bails out thinking it can't determine the type.Problem 2: Rule throws errors in weird cases
A 2nd issue is that the rule assumes that a definition is either a
Variable,ImportBinding, orParameter.Those are the only ones that make sense in this context, but other definition types exist -
FunctionName,ClassName,CatchClause(plus 4 more in TS-ESLint fortypeetc).Because of the assumption that after handling
VariableandImportBinding, the definition must beParameter, weird code like this would cause a hardTypeError: Cannot read properties of undefinedcrash:definition.node.params[definition.index]assumesnodehas aparamsproperty, which it doesn't in this case.Obviously code like this is just plain stupid, but nonetheless, it's not desirable for lint rules to throw errors in any circumstances, as it causes ESLint to immediately exit without finishing linting other files.
This PR
This PR fixes both problems, by replacing
definition.node.params[definition.index]withdefinition.name.parent.definition.nameis theIdentifiernode for the parameter, and for a parameter with a default value likems = 1, its parent is theAssignmentPatternnode. This is a more direct way to get at the same information, and doesn't rely on the deprecatedindexproperty.Additionally, this also handles destructured parameters with numeric defaults, which the rule previously could not detect:
This could be considered a breaking change, but it feels in the spirit of the rule to catch these cases.
If any change to behavior is undesirable, please say so, and I can revise the PR to also check that
name.parent.parentis the function parameter. This would make this PR entirely equivalent to how it was before, except for fixing the edge caseTypeError.Note
Medium Risk
Changes lint rule behavior to flag more
cy.waitpatterns (including destructured parameter defaults) and alters scope-resolution logic, which could introduce new lint failures or missed edge cases but is limited to static analysis.Overview
Fixes
no-unnecessary-waiting’s identifier-default detection by switching fromdefinition.node.params[definition.index]todefinition.name.parentwhen determining whether an identifier comes from a parameter with a numeric default.This avoids crashes on non-parameter definition types and expands detection to destructured parameters (object/array/nested) with numeric defaults; tests are updated with new valid/invalid coverage for these cases and for function/class/catch bindings.
Written by Cursor Bugbot for commit 5e9d9fe. This will update automatically on new commits. Configure here.