Fix readable stream as body check on safari #12
Conversation
|
|
||
| // If fails to handle fetch(Request), it's likely not a native implementation of | ||
| // Fetch API, so it's not supported. | ||
| await fetch(request.clone()); |
There was a problem hiding this comment.
Hey @bugii , thank you for contributing with this great alternative.
There's 2 usages we are trying to assess here, one is browsers and one is Node.js with patched or non-native Fetch API implementations.
This line specifically designed to fail with patched fetch that do not support fetch(Request) and instead just assumes fetch(uri, options). We have seen this in few circumstances one of which was the ElizaOS monorepo.
There was a problem hiding this comment.
Hey @cesarenaldi, thank you for the review! Was late at night and I overlooked that code somehow :D
I added the fetch call again.
src/utils.ts
Outdated
| function detectStreamSupport() { | ||
| let duplexAccessed = false; | ||
|
|
||
| const hasContentType = new Request('data:text/plain;charset=utf-8,42', { |
There was a problem hiding this comment.
Compared to the example in the google page you linked, here we are using a data URI, which includes a mime type. Wondering if this could make a difference, specifically with the hasContentType check.
There was a problem hiding this comment.
Sadly I cannot use an empty string as in the example (gives an error in node - which makes sense I guess since relative paths in servers are not really a thing). I think this one is safer, even though I would be very much surprised if the request url had an impact on the content type of the outgoing request before actually fetching it.
data url in the support check
bugii
force-pushed
the
fix/readable-stream-support-check-safari
branch
from
98ee09a to
8f17913
Compare
| ); | ||
| const body = await res.text(); | ||
|
|
||
| return duplexAccessed && !hasContentType && body === '\x00'; |
There was a problem hiding this comment.
The intent with the original check was to verify that the testStream was consumed by checking its text representation. A buffer with 0 byte in it renders the \x00 char.
Since the 'response' of a Data URI request is the data URI payload itself, would this be a string with '42' in this case and not match the \x00?
There was a problem hiding this comment.
I'll take this PR for a spin and see if we can adjust it.
|
@bugii this is now available in |
I am getting the same error on Safari iOS as well as macOS when trying to upload a file to grove:
According to MDN, Safari actually really does not support ReadableStreams as body inside fetch requests.
Thus, I figured the support check inside this package is not correct and I was correct: The detectStreamSupport method returns true on Safari but then the actual request fails. I found a nice article from google where they have another method of testing for support: https://developer.chrome.com/docs/capabilities/web-apis/fetch-streaming-requests#feature_detection
I think that method is quite nice as its not async, too.
Tested on:
Firefox: both versions return
falseChrome: both versions return
trueSafari: new version returns
false, old versiontrue