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

feat: Allow cy.visit to visit cross origin sites. #23297

Merged
merged 79 commits into from
Sep 15, 2022

Conversation

mjhenkes
Copy link
Member

@mjhenkes mjhenkes commented Aug 11, 2022

User facing changelog

Additional details

This feature enables users to visit cross origin sites without having to place the visit inside of a cy.origin block

cy.origin blocks no longer have to immediately follow navigation events

Navigating to a cross origin page will no longer have up to a 300 ms delay.

'window:load' must now be declared for a cy.origin block before a navigation event occurs.

we also no longer have the long page load timeout when you navigate to a cross origin page but don't follow up with a cy.origin block

Steps to test

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@mjhenkes mjhenkes added the topic: cy.origin Problems or enhancements related to cy.origin command label Aug 11, 2022
@mjhenkes mjhenkes self-assigned this Aug 11, 2022
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 11, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Aug 12, 2022



Test summary

4773 0 373 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 04a8290
Started Sep 15, 2022 5:15 PM
Ended Sep 15, 2022 5:30 PM
Duration 14:44 💡
OS Linux Debian - 11.3
Browser Electron 102

View run in Cypress Dashboard ➡️


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

timeoutId = setTimeout(() => {
this.off(event, handler)
reject()
}, 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think an error message would be a good idea, even if the caller ends up ignoring it. Just in case this error leaks out somehow, it will be much easier to pinpoint the issue with an error message.

packages/driver/src/cross-origin/cypress.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/navigation.ts Outdated Show resolved Hide resolved
reject(new Error(`${event} failed to receive a response from ${originPolicy} spec bridge within 1 second.`))
}, 1000)

this.once(event, handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if 2 (or more) of the same event are sent in succession, but resolved in the primary in a different order? Won't the handlers get switched? It might not be a problem with current usage of this, but I worry it's a foot-gun for future uses.

I'd expect there to be a unique ID passed with the message to the primary and it's only resolved here when the same ID is responded to.

Copy link
Member Author

Choose a reason for hiding this comment

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

UUID is a good idea. Let me work on that.

packages/driver/cypress/e2e/e2e/origin/origin.cy.ts Outdated Show resolved Hide resolved
timeoutId = setTimeout(() => {
this.off(event, handler)
reject()
}, 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

with the error now I am wondering if we just want to have a variable like const promisifiedTimeout = 1000 and just use a formatted string for the error so if we change it in the future we just do it it one place.

packages/driver/src/cross-origin/cypress.ts Show resolved Hide resolved
packages/driver/src/cy/commands/origin/index.ts Outdated Show resolved Hide resolved
packages/runner/injection/patches/cookies.js Outdated Show resolved Hide resolved
packages/runner/injection/cross-origin.js Show resolved Hide resolved
@AtofStryker AtofStryker self-requested a review September 15, 2022 15:05
@chrisbreiding chrisbreiding self-requested a review September 15, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cy.origin Problems or enhancements related to cy.origin command
Projects
None yet
3 participants