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.

Sign up for GitHub

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

Correct window type #6624

Closed
wants to merge 10 commits into from
Closed

Correct window type #6624

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
04724b2
Correct window type
OliverJAsh Mar 3, 2020
a8644df
Upgrade minimum TS version from 2.9 to 3.4
OliverJAsh Mar 3, 2020
52254ac
Add test
OliverJAsh Mar 3, 2020
c50ce4b
Format
OliverJAsh Mar 3, 2020
36b7d47
Test type
OliverJAsh Mar 10, 2020
9b0910a
Merge branch 'develop' into patch-3
OliverJAsh Mar 10, 2020
c314185
Expect type
OliverJAsh Mar 18, 2020
50ea3bc
Fix syntax error
OliverJAsh Mar 18, 2020
19c6a24
Merge branch 'develop' into patch-3
OliverJAsh Mar 24, 2020
a588b9d
Merge branch 'develop' into patch-3
jennifer-shehane Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// Mike Woudenberg <https://github.com/mikewoudenberg>
// Robbert van Markus <https://github.com/rvanmarkus>
// Nicholas Boll <https://github.com/nicholasboll>
// TypeScript Version: 2.9
// TypeScript Version: 3.4
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a spooky/major change? Are we sure this isn't going to impact folks downstream maybe using earlier TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed to support globalThis. It may well affect your users, so you might want to put this PR on hold until more of your users are on 3.4 or above.

Copy link
Member

Choose a reason for hiding this comment

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

We've run into this before, so this is not the first time this has been suggested. #4950 (review)

Copy link
Contributor

@CypressJoseph CypressJoseph Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Since we just accepted a pull to move to 3.0, I'm guessing that's going to be a conflict -- curious if these changes work at that level (3.0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3.4 is the minimum version if we want to use globalThis

// Updated by the Cypress team: https://www.cypress.io/about/

/// <reference path="./cy-blob-util.d.ts" />
Expand Down Expand Up @@ -1995,7 +1995,7 @@ declare namespace Cypress {
})
```
*/
window(options?: Partial<Loggable & Timeoutable>): Chainable<Window>
window(options?: Partial<Loggable & Timeoutable>): Chainable<Window & typeof globalThis>

/**
* Scopes all subsequent cy commands to within this element.
Expand Down
6 changes: 6 additions & 0 deletions cli/types/tests/kitchen-sink.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ const obj = {
}
cy.spy(obj, 'foo').as('my-spy')

cy.window().then(window => {
const windowTest: Window & typeof globalThis = window

window.eval('1')'
})

// clearLocalStorage signatures
cy.clearLocalStorage()
cy.clearLocalStorage('todos')
Expand Down