fix: UNIFY-676 browsers should be configurable in setupNodeEvents

[tgriesser]

• Closes UNIFY-676

Adds a system test to show that filtering browsers is working properly & $ExpectType assertion for dtslint to assert the correct type, @JessicaSachs' original issue

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[cypress]

---

Test summary

18015 • 0 • 229 • 0 • 1

---

Run details

Project	cypress
Status	Passed
Commit	da6eca1
Started	Feb 28, 2022 8:47 PM
Ended	Feb 28, 2022 8:59 PM
Duration	11:45 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	cypress/e2e/commands/net_stubbing.cy.ts		1
1	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[jennifer-shehane]

@tgriesser There's a failure in the types:

lib/browsers/chrome.ts(407,66): error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[tbiethman]

We just need to interpolate the majorVersion now:

cypress/packages/server/lib/browsers/chrome.ts

Line 407 in a015397

if (CHROME_VERSIONS_WITH_BUGGY_ROOT_LAYER_SCROLLING.includes(majorVersion)) {

[tgriesser]

I had updated the type because it seemed to be wrong but who knows, the types in the server layer are pretty tough to follow. Didn't need to address it as part of this PR.

[JessicaSachs]

(whoops, meant to request changes. I really wish you could delete these things)

[JessicaSachs]

Doesn't work, Tim and I DM'd about it already.

[elevatebart]

Would this mean that now config.browser is available in the types of setupNodeEvents()?

The value will always be null and setting it should have no effect.

How far off am I ?

[lmiller1990]

Seems fine to me, I followed https://docs.cypress.io/guides/guides/launching-browsers#Customize-available-browsers and got this:

I also fixed the typing errors in an effort to get this over the line: 97ba5cf

[lmiller1990]

Alrighty I got this green, let's get one more test/approval. The types are working fine for me, too, from what I can see. I had to specify headless and isHeaded which seems weird, but fixing the types in the entirely of Cypress is a huge task.

[tgriesser]

Changing to draft to investigate why the $ExpectType types aren't being checked

[tgriesser]

@elevatebart:

Would this mean that now config.browser is available in the types of setupNodeEvents()?

Nah, it's using a Partial so it'll be Browser | undefined

[lmiller1990]

Seems fine to me! I tried it as mentioned in a previous review and it worked okay, including types.

[elevatebart]

@tgriesser I beg to disagree, the problem is still here:
The code below seems to be valid typed configFile.

import { defineConfig } from 'cypress'

export default defineConfig({
  e2e: {
    setupNodeEvents (on, config) {
      config.browser = 'chrome'
    },
  },
})

What I am saying is that the types are not correct since they contain more than the actual object. Instead of helping the types we have here could confuse users.
I prefer that we do not fix it in this PR though.

[lmiller1990]

@elevatebart I think what Tim means is since it's a Partial everything is going to be nullable, which seems fine - users will basically need to assume any of the properties they are accessing are nullable and do the required defensive checks.

I agree the idea would be the exact types (no nullable) but like you said we cannot do this right now, it's pretty complex and outside the scope of this PR.

Edit: looks like we are on the same page, you've approved too - definitely good to improve the types more in the future.