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: transfer response buffer on safari #1771

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Oct 15, 2023

Motivation

Fixes an issue where sending a ReadableStream via messageChannel.port1.postMessage() throws the following error on Safari:

DataCloneError: The object can not be cloned.

Solution

MSW will run a quick supports check for ReadableStream as transferable. If it fails, MSW will read the mocked response's ArrayBuffer body and send it at once to the worker. This still means that mocking streams won't work in Safari as reading the response body will exhaust its stream so it loses the chunks/timings information when the client will receive it.

That's acceptable since it's an issue in Safari and must be addressed there.

* @note Safari doesn't support transferring a "ReadableStream".
* Check that the browser supports that before sending it to the worker.
*/
if (supportsReadableStreamTransfer()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I dislike checking this on every response listener.

This should be determined once because its value is bound to the current browser and cannot change on runtime.

We should move this somewhere to the setupWorker() call, I think. It's computed once, even before handling the first response, and stored in the worker context. Something like worker.context.supportsReadableStreamTransfer can work.

)
} else {
// As a fallback, send the response body buffer to the worker.
const responseBuffer = await responseClone.arrayBuffer()
Copy link
Member Author

Choose a reason for hiding this comment

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

@thepassle, would this work for your use case if we read the mocked response body as ArrayBuffer instead of Blob? I find it to be more correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it works:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome!

In the future, we should print a warning if the developer is mocking a ReadableStream body while transferring that stream isn't supported by their browser. We will still use the fallback to ArrayBuffer but the developer may be confused lest we show a warning of what's happening.

useFallbackMode:
!('serviceWorker' in navigator) || location.protocol === 'file:',
supports: {
serviceWorkerApi:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good chance to move useFallbackMode to supports.serviceWorkerApi since now we check for two things on the side of browser's support.

@thepassle
Copy link
Contributor

tried this out locally and it seems to work, lgtm

@kettanaito kettanaito merged commit 6405bea into feat/standard-api Oct 15, 2023
9 checks passed
@kettanaito kettanaito deleted the fix/stream-transfer-safari branch October 15, 2023 14:03
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.

None yet

2 participants