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: Properly type Cypress and Chai globals as properties of the global object #27540

Merged
merged 12 commits into from Aug 24, 2023

Conversation

rewento
Copy link
Contributor

@rewento rewento commented Aug 11, 2023

Minor modifications to CLI types so that Cypress and Chai globals are correctly declared not only to be in the global scope but also to be in properties of globalThis, which they are. The change allows composers to reference them through the global object (e.g., 'globalThis.cy.mount', 'globalThis.expect', 'window.assert'). The current typing prohibits this by incorrectly declaring that the objects in question are held in local variables of the global scope rather than in properties of the global object.

Additional details

  • Why was this change necessary?
    This small change accommodates a coding style convention that demands that values in properties of the global object be accessed via the globalThis property of that object to enhance clarity regarding the origin of the values as globals as opposed to values defined in and imported from modules.
  • What is affected by this change?
    The change has no impact on the TypeScript compiler's acceptance of code that uses Cypress globals without that convention, which is the majority of such code.
  • Any implementation details to explain?
    Resolving the issue entailed swapping the use of const for var in type declarations in cli/types/cypress-expect.d.ts and cli/types/cypress-global-vars.d.ts.

Steps to test

Ensure that typical code that leverages Cypress like this is still is accepted by the TypeScript compiler:

const browserVersion = window.Cypress.browser.version;
window.cy.mount(AppComponent);
window.assert("foo" !== "bar", "foo is not bar");
window.expect(true).not.to.be.false;

Then confirm that the TypeScript compiler also issues no errors regarding code that follows the aforementioned convention like the following:

const browserVersion = window.Cypress.browser.version;
window.cy.mount(AppComponent);
window.assert("foo" !== "bar", "foo is not bar");
window.expect(true).not.to.be.false;
const browserVersion = globalThis.Cypress.browser.version;
globalThis.cy.mount(AppComponent);
globalThis.assert("foo" !== "bar", "foo is not bar");
globalThis.expect(true).not.to.be.false;

How has the user experience changed?

Before:
image

After:
image

PR Tasks

  • [na] Have tests been added/updated?
  • [no] Has a PR for user-facing changes been opened in cypress-documentation? (Little to no value in drawing attention in the documentation to the fact that globalThis.cy and window.cy now work just as well as cy in TypeScript code.)
  • [yes] Have API changes been updated in the type definitions?

… correctly declared not only to be in the global scope but also to be in properties of globalThis, which they are. The change allows composers to explicitly access them through globalThis (e.g., 'globalThis.cy.mount', 'globalThis.expect'). The current typing prohibits this by incorrectly declaring that the objects in question are held in local variables of the global scope rather than in properties of globalThis.
@CLAassistant
Copy link

CLAassistant commented Aug 11, 2023

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@rewento rewento marked this pull request as ready for review August 11, 2023 23:36
@cypress
Copy link

cypress bot commented Aug 12, 2023

35 flaky tests on run #50391 ↗︎

0 28021 1355 0 Flakiness 35

Details:

Merge branch 'release/13.0.0' into update-cypress-request-version
Project: cypress Commit: 27f9b3b7d9
Status: Passed Duration: 23:08 💡
Started: Aug 24, 2023 2:41 PM Ended: Aug 24, 2023 3:04 PM
Flakiness  specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs Output Screenshots Video
Flakiness  cypress-origin-communicator.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
Cypress In Cypress Origin Communicator > cy.origin passivity with app interactions > passes upon test reload mid test execution Output Screenshots Video
Flakiness  e2e/origin/basic_login.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test Artifacts
basic login > visit primary first > logs in with idp redirect Output Video
Flakiness  e2e/origin/commands/unsupported_commands.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test Artifacts
cy.origin unsupported commands > cy.origin() is not yet supported Output Video
Flakiness  e2e/origin/commands/misc.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test Artifacts
cy.origin misc > .end() Output Video

The first 5 flaky specs are shown, see all 23 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Could you add tests for this in cypress-tests.ts? Adding the following should suffice:

namespace CypressGlobalsTests {
  Cypress
  cy
  expect
  assert

  window.Cypress
  window.cy
  window.expect
  window.assert

  globalThis.Cypress
  globalThis.cy
  globalThis.expect
  globalThis.assert
}

@nagash77 nagash77 self-assigned this Aug 18, 2023
@rewento
Copy link
Contributor Author

rewento commented Aug 18, 2023

@chrisbreiding

I'll soon push a commit on to the source branch of this PR that adds the code you provided in your comment to the cypress-test.ts file verbatim.

However, the JavaScript compiled from that added test code would execute with no runtime error with or without the Cypress and Chai globals type changes introduced in this PR. That being said, the TypeScript compiler would refuse to compile cypress-tests.ts in the first place without those type adjustments.

So is that a good enough addition to the tests? That the added test logic, while incapable of triggering a runtime error under any circumstances, would outright prevent the compilation (and thus execution) of the test code itself without the Cypress and Chai globals type changes in question?

@chrisbreiding
Copy link
Contributor

So is that a good enough addition to the tests? That the added test logic, while incapable of triggering a runtime error under any circumstances, would outright prevent the compilation (and thus execution) of the test code itself without the Cypress and Chai globals type changes in question?

Yes, we have a CI job that runs the TypeScript compiler on that file, so it stands as a test for the TypeScript types. Since it fails without these type additions, it's a perfectly good test that they're doing what we expect them to do. Thanks for adding them!

@chrisbreiding chrisbreiding self-requested a review August 18, 2023 18:00
@nagash77 nagash77 merged commit 4288634 into cypress-io:develop Aug 24, 2023
79 of 80 checks passed
@nagash77 nagash77 removed their assignment Aug 24, 2023
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 29, 2023

Released in 13.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.0.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In TypeScript, allow referencing Cypress and Chai globals as properties of the global object.
5 participants