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(xhr-api): Handle "blob" response types in intercepted XHR requests #151

Conversation

tehhowch
Copy link
Contributor

Closes #144

I believe this implementation does require dropping all support for internet explorer, since the async/await syntax won't be supported by its JS runtime.

@tehhowch
Copy link
Contributor Author

@christian-bromann @chmanie thoughts?

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chmanie
Copy link
Member

chmanie commented Oct 25, 2021

I guess the dropped support for IE should be mentioned somewhere, also this is a breaking change and should require a major version bump.

@tehhowch
Copy link
Contributor Author

tehhowch commented Oct 25, 2021

I guess the dropped support for IE should be mentioned somewhere, also this is a breaking change and should require a major version bump.

@chmanie I wasn't sure the implementation would be amenable, so I hadn't made those changes. I'm not sure if there's a single way to convert blob response types in IE and in modern browsers as well, perhaps there is some solution using FileReader and another callback level.

Alternately, if a Babel transpilation step were used, the use of modern JavaScript could be applied throughout the library without sacrificing compatibility. Adding a Babel transpile step isn't really the point of this PR, so I didn't want to pollute its purpose with another change.

edit: I filed #152 (comment) as a way to help improve this process (though first-time contributors such as myself won't be notified until CICD stage is manually started).

@christian-bromann
Copy link
Contributor

I guess the dropped support for IE should be mentioned somewhere

Are we breaking IE because of the use of async/await? I don't mind avoiding it in favor of some indentation.

@tehhowch
Copy link
Contributor Author

I guess the dropped support for IE should be mentioned somewhere, also this is a breaking change and should require a major version bump.

@chmanie It's worth noting that if a website has polyfilled fetch, but not also polyfilled Promise.all, then IE fetch interception is broken for that website (which could be the cause of #105, though details are severely lacking there), and this has been the case since 56db13c.

@tehhowch
Copy link
Contributor Author

@chmanie should be IE-compatible now; i replaced async + await with a FileReader callback

@christian-bromann
Copy link
Contributor

@tehhowch mind rebasing this branch?

 - Account for CRLF line endings instead of LF, and also changes to the file's contents
The test fails comparing the body, as Node receives `{}` for any blob instead of the true content.
 - update event listener to support deferred body parsing
 - When a `Blob` response is detected, use a FileReader to convert it into an `ArrayBuffer` and then decode as text.
   - Modern browsers could use `async` functions to await the result of `Blob.arrayBuffer()`, but the `FileReader.readAsArrayBuffer` approach preserves the existing state of Internet Explorer compatibility.
@tehhowch tehhowch force-pushed the features/parse-blob-response branch from 722330c to bfe1631 Compare October 29, 2021 13:16
@tehhowch
Copy link
Contributor Author

@christian-bromann done!

@christian-bromann christian-bromann merged commit ceca67e into webdriverio-community:main Oct 29, 2021
@tehhowch tehhowch deleted the features/parse-blob-response branch October 29, 2021 15:38
@tehhowch
Copy link
Contributor Author

tehhowch commented Nov 3, 2021

@christian-bromann can we release 4.2.0? tests are passing on main

@christian-bromann
Copy link
Contributor

Sure, thanks for pinging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reading XHR requests with "blob" responseType
3 participants