feat(upload-client): Remove ipfs-utils #1627
Conversation
talentlessguy
force-pushed
the
remove-ipfs-utils
branch
from
5192b11 to
f00abea
Compare
| const preH2 = protocol !== 'h2' && protocol !== 'h2c' && protocol !== 'h3' | ||
|
|
||
| /* c8 ignore next 3 */ | ||
| if ((isBrowser || preH2) && !isNode) { |
There was a problem hiding this comment.
The test for isBrowser is an existence check for XHR, which could be false, but presumably preH2 could be true and we could also not be in Nodejs...
We should not drop in here if XHR is not available.
There was a problem hiding this comment.
This might get confusing so I'll list the conditions/gotchas here:
-
Node.js fetch aka undici by default doesn't directly support HTTP/2, but supports body streaming either way.
-
Browser fetch supports streaming only in HTTP/2 (AFAIK)
-
Deno fetch is the same
-
Bun doesn't support HTTP/2 yet
-
All browsers implement XHR, so it's a safe assumption
perhaps a better set of conditions could be handled, not sure which one to handle all these cases (given that for some platforms streaming is available in both. for some in none and for others only in one)
also, in order to test this more thoroughly, a test case with HTTP/2 server perhaps should be added, but I haven't written one yet (should be trivial enough)
packages/upload-client/src/types.ts
Outdated
| body: Exclude<IpfsUtilsFetchOptions['body'], import('node:stream').Readable> | ||
| /** | ||
| * Can be passed to track upload progress. | ||
| * Note that if this option in passed underlying request will be performed using `XMLHttpRequest` and response will not be streamed. |
There was a problem hiding this comment.
Note: Response not streaming is not an issue - we don't download anything big in the client.
This PR removes redundant ipfs-utils for the upload-client which only uses it in one place. I rewrote the code from the original ipfs-utils to use native fetch and be platform-agnostic.
pnpm testpassed, while preserving 100% code coverage.Tested on: