search

muxinc / upchunk

Uploads Chunks! Takes big files, splits them up, then uploads each one with care (and PUT requests).
MIT License
340 stars 46 forks source link

feat: add opt-in fallback support (to roughly old impl) for readablestream errors. #138

Closed cjpillsbury closed 5 months ago

cjpillsbury commented 5 months ago

This PR adds an "opt in" (via config options) fallback for cases where the (newer) ReadableStream File/Blob API fails (e.g. due to https://bugs.webkit.org/show_bug.cgi?id=272600). When "opted in", upchunk will revert to reading the file into memory, which is less ideal in general but may be preferred to simply failing and notifying (via an error event) about the failure. This allows developers to decide how they want to handle these cases.

The fallback can be enabled by setting the useFileSliceFallback options flag to true.

fixes #134

NOTE: The reason this isn't being turned on by default relates to the primary known error that necessitates the fallback. WebKit (including Safari) has a bug where, if a file is 4GB or larger, it will error when attempting to read the file using the File's ReadableStream. However, we migrated to using ReadableStream instead of loading the file into memory in v3.x specifically because of concerns/complaints that this could be very bad for JS thread heap memory consumption, sometimes resulting in crashes for memory-scarce environments or excessively large files. Since 4GB isn't an excessively small file (😅), and this is the case we are primarily accounting for, we didn't want to suddenly and automatically put developers back into that "greedy memory" usage scenario, but, since this issue has yet to be resolved in WebKit, we want to provide some answer to those cases beyond notifying an error (which we now do, too).

mmcc commented 5 months ago

Approved, but feels like we should update the documentation in this PR too? At the very least make sure it's included in the list of options.

cjpillsbury commented 5 months ago

Approved, but feels like we should update the documentation in this PR too? At the very least make sure it's included in the list of options.

Yup good call! Will do before merging.

cjpillsbury commented 5 months ago

Went ahead and spruced up some other docs bits and bobs that I noticed.