-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
automatically polyfill window.fetch with XMLHttpRequest #7597
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
Test summaryRun details
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super succinct. I read through unfetch
. Nice find!
I have one nit which is that I'd really love to avoid toString + eval
. I wrote notes on how we might use the proper XMLHttpRequest object. LMK if that makes sense.
packages/driver/src/cypress/cy.js
Outdated
// drop "fetch" polyfill that replaces it with XMLHttpRequest | ||
// by using "eval" we force the polyfill to use XMLHttpRequest objects | ||
// from the app iframe that we wrap for network stubbing | ||
contentWindow.eval(`fetch=${fetchPolyfill}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can pretty easily avoid eval + toString and use the correct XMLHttpRequest object if we patch-package and take in the XMLHttpRequest (or content window) reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ughh, but that would be more complex to set up and maintain. Remember the has-binary2 disaster
Note: the webpack bundles the following file
which in the |
User facing changelog
Cypress will automatically replace
window.fetch
with polyfill that drops down to XMLHttpRequest object that can be intercepted using built-in Network APIAdditional details
fetch
methodsHow has the user experience changed?
Before if the web application used
fetch
, the user had to set up polyfill / deletewindow.fetch
on each window load in order to use our network features. With this feature, the user can simply set incypress.json
experimentalFetchPolyfill: true
and Cypress automatically polyfillswindow.fetch
viaXMLHttpRequest
which we stubOpen questions
fetch
behavior. For example, it does not support streaming responses... If we automatically polyfill nativefetch
, web apps that do not even need network support suddenly would stop working inside Cypress. Answer: YesPR Tasks
cypress-documentation
? Add experimentalFetchPolyfill docs cypress-documentation#2881cypress.schema.json
?