Skip to content

fix: no-unnecessary-waiting flag identifiers defined in object/array patterns#308

Merged
mschile merged 1 commit intocypress-io:masterfrom
overlookmotel:fix-no-unnecessary-waiting
Apr 6, 2026
Merged

fix: no-unnecessary-waiting flag identifiers defined in object/array patterns#308
mschile merged 1 commit intocypress-io:masterfrom
overlookmotel:fix-no-unnecessary-waiting

Conversation

@overlookmotel
Copy link
Copy Markdown
Contributor

@overlookmotel overlookmotel commented Mar 20, 2026

Fix 2 related but separate problems with no-unnecessary-waiting rule.

Problem 1: Incompatibility with TS-ESLint and Oxlint

I came across this problem when a contributor added eslint-plugin-cypress to Oxlint's conformance testing. oxc-project/oxc#20265. We noticed that 3 tests for this rule were failing.

no-unnecessary-waiting rule uses definition.index to look up parameter nodes when checking if a function parameter has a numeric default value. This property is part of eslint-scope's Definition class, but it's listed as deprecated in ESLint's Scope Manager Interface docs. For this reason, TS-ESLint's scope manager doesn't implement the index property 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:

function customWait(ms = 1) { cy.wait(ms) }
const customWait = (ms = 1) => { cy.wait(ms) }

In TS-ESLint or Oxlint, definition.index is undefined, so definition.node.params[undefined] returns undefined, 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, or Parameter.

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 for type etc).

Because of the assumption that after handling Variable and ImportBinding, the definition must be Parameter, weird code like this would cause a hard TypeError: Cannot read properties of undefined crash:

class HaHaHaGotYou {}
cy.wait(HaHaHaGotYou)

definition.node.params[definition.index] assumes node has a params property, 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] with definition.name.parent.

definition.name is the Identifier node for the parameter, and for a parameter with a default value like ms = 1, its parent is the AssignmentPattern node. This is a more direct way to get at the same information, and doesn't rely on the deprecated index property.

Additionally, this also handles destructured parameters with numeric defaults, which the rule previously could not detect:

function customWait({ ms = 1 }) { cy.wait(ms) }
function customWait([ms = 1]) { cy.wait(ms) }
function customWait({ opts: { ms = 1 } }) { cy.wait(ms) }

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.parent is the function parameter. This would make this PR entirely equivalent to how it was before, except for fixing the edge case TypeError.


Note

Medium Risk
Changes lint rule behavior to flag more cy.wait patterns (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 from definition.node.params[definition.index] to definition.name.parent when 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.

Copilot AI review requested due to automatic review settings March 20, 2026 00:24
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 20, 2026

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Copy Markdown

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 using definition.name.parent to avoid deprecated/missing index.
  • 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.

@mschile mschile merged commit f5ccf77 into cypress-io:master Apr 6, 2026
19 of 20 checks passed
@cypress-app-bot
Copy link
Copy Markdown

🎉 This PR is included in version 6.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mschile
Copy link
Copy Markdown
Contributor

mschile commented Apr 6, 2026

@overlookmotel 👋🏼, thanks for the contribution! Sorry for taking so long to take a look at it!

@overlookmotel overlookmotel deleted the fix-no-unnecessary-waiting branch April 15, 2026 13:54
graphite-app bot pushed a commit to oxc-project/oxc that referenced this pull request Apr 15, 2026
…1466)

Bump `eslint-plugin-cypress` in Oxlint JS plugin conformance tests.

Now that cypress-io/eslint-plugin-cypress#308 is merged, 100% tests pass rate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants