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

fix(types): fix waitForSelector typing to not union null when appropriate #6344

Merged
merged 1 commit into from May 5, 2021

Conversation

jharwig
Copy link
Contributor

@jharwig jharwig commented Apr 28, 2021

I found an issue with the types.d.ts file regarding waitForSelector when passing some options, but no state. When no state key is provided, the return type would include null. This is despite the default state value is visible and shouldn't allow null.

I discovered when doing something like below. The example shows passing no options gives the correct return type, but not when options are passed.
Screen Shot 2021-04-27 at 7 08 06 PM

Source Code
  const el = await page.waitForSelector(tid('document-viewer'), {
    timeout: docLoadTimeout, // no state passed, should default to visible
  })
  await el.click() // Object is possible 'null'

  const elNoOptions = await page.waitForSelector(tid('document-viewer'))
  await elNoOptions.click()

I found the solution, added some tests, but I also fixed the canBeNull assertion because my new test case wouldn't fail the tests as they were (even though the types clearly included null, but the assertion was missing it.) Looking at some code review comments this might have been the case for a while. If anyone has some insight, I'd love to hear it.

…iate

Previously when options were defined, but no `state` key was provided,
the types would return null as an option. Even though the default state
is `visible` and shouldn't allow `null`.

Tests updated to fail appropriately and new tests added for this case.
@ghost
Copy link

ghost commented Apr 28, 2021

CLA assistant check
All CLA requirements met.

@JoelEinbinder JoelEinbinder self-assigned this Apr 30, 2021
Copy link
Contributor

@JoelEinbinder JoelEinbinder left a comment

Choose a reason for hiding this comment

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

Great patch. Thanks!

@JoelEinbinder JoelEinbinder merged commit 42a5566 into microsoft:master May 5, 2021
@jharwig jharwig deleted the fix-waitForSelector-types branch May 5, 2021 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants