Explicitly passing { timeout: undefined } should be treated the same as not passing it at all

[ashleymercer]

Current behavior

Explicitly passing timeout: undefined results in an error for several methods:

// 1. Empty options (timeout is "implicitly" undefined): works
cy.get('foo', {})
// 2. Timeout is explicitly a number: works
cy.get('foo', { timeout: 1000 })
// 3. Timeout is explicitly undefined: doesn't work
cy.get('foo', { timeout: undefined })

Desired behavior

It looks like #29323 was the change that introduced this error checking, and while I agree that we shouldn't be able to pass e.g. random objects as the timeout (which doesn't make any sense) I believe it should be possible to pass explicit undefined and that this should be treated the exact same as not passing a value at all.

My understanding is that in (almost?) all cases, a value being "implicitly" undefined (by not being defined on an object) vs "explicitly" passed e.g. as timeout: undefined should be treated the same.

Additionally, the type signature of these methods uses Partial<Timeoutable>, which won't distinguish between the explicit and implicit cases e.g. (trimmed for clarity):

get(selector: string, options?: Partial<Timeoutable>): Chainable<JQuery<E>>

The TypeScript checked / IDEs etc therefore doesn't warn when passing undefined explicitly and we only get an error at runtime, which is quite annoying.

Test code to reproduce

cy.get('div', { timeout: undefined })

Cypress Version

13.12.0

Node version

20.12.1

Operating System

Windows Server 2016

Debug Logs

No response

Other

The motivation for this change is very similar to #29425 - we also use the page object pattern and have a lot of custom methods / configuration where the timeout can (optionally) be specified, and is then passed through to Cypress.

However, unlike that ticket, I'm not proposing a change to anything other than explicit { timeout: undefined }. In the case where it is passed explicitly like this, it should be treated the same as if it hadn't been passed at all.

[jennifer-shehane]

This would be a breaking change to introduce undefined as defaulting back to the default timeout.

As a workaround today, you could use this in some form when things are undefined

  it('test', () => {
    cy.get('foo', { timeout: Cypress.config('defaultCommandTimeout') })
  });

[ashleymercer]

@jennifer-shehane thanks for the quick response.

This would be a breaking change to introduce undefined as defaulting back to the default timeout.

Ah maybe I've gotten the wrong end of the stick. My understanding was that previously (prior to #29323) these two lines of code did the exact same thing:

cy.get('foo', {})
cy.get('foo', { timeout: undefined })

Is that correct? Or was it the case that they previously did something different?

If they did previously do the same thing, then my request is simply that we revert to that behaviour (I don't actually have strong feelings about what that behaviour is - no timeout, use default timeout, throw an exception - I only care that the behaviour be consistent).

[jennifer-shehane]

Prior to that PR, some values passed to the timeout option would cause the test to hang (like {} or "500"). Passing undefined however did default to the default timeout and since that PR invalid values now fail the test. Undefined was never documented or coded with any expectation on its behavior. So treated it as invalid and erroring was intended to surface areas where users may have mistakenly passed a value.

To change the behavior back to allowing undefined as a value and defining a behavior would be a breaking change in behavior - since users expect the test to throw an error if undefined is passed and it would then do another behavior.

We'll consider this change for any upcoming breaking change release and leave this ticket open to collect feedback.

[ashleymercer]

Ah thanks for the (very in-depth) explanation 😄 that makes a lot more sense now.