feat: add opt-in fallback support (to roughly old impl) for readablestream errors. #138
Conversation
| @@ -150,6 +158,95 @@ export class ChunkedStreamIterable implements AsyncIterable<Blob> { | |||
| } | |||
| } | |||
|
|
|||
| export class ChunkedFileIterable implements ChunkedIterable { | |||
There was a problem hiding this comment.
NOTE: There's some amount of (fairly simple) redundancy between ChunkedFileIterable and ChunkedStreamIterable (i.e. almost everything that is not captured in the async generator/iterable function). We could create a common base class, but wasn't sure if that was a significant enough value add. Open to having pushback on this, though.
There was a problem hiding this comment.
Said this in Slack, but I'm personally fine with this explicitly being a band-aid. We want to delete this code if Safari ever patches the issue, so I don't see any reason to spend more time than we have to on the fallback.
src/upchunk.ts
Outdated
| @@ -290,20 +390,46 @@ export class UpChunk { | |||
| this.success = false; | |||
| this.nextChunkRangeStart = 0; | |||
|
|
|||
| if (options.useFileSliceFallback) { | |||
There was a problem hiding this comment.
This basically monitors for an error event and then checks if there is also a corresponding error on the current instance of this.chunkedIterable. We could instead add concepts like error types/categories info to the event itself if this feels a bit too "acrobatics-y". Larger refactors to capture this logic in sendChunks() felt like the wrong path forward, especially if we want to treat this as a "stopgap" as much as possible.
Also, how do folks feel about useFileSliceFallback for a name? I'm amenable to changes here, since "Names Are Hard™️"
There was a problem hiding this comment.
Explicitly calling it a workaround may help us/contributors remember why it's like this. How would you feel about useLargeFileWorkaround or something?
| // Retry using ChunkedFileIterable, which reads the file into memory instead | ||
| // of a stream. | ||
| if (this.chunkedIterable.error) { | ||
| console.warn( |
There was a problem hiding this comment.
We "swallow the error" if we're falling back, but still log a warning for folks, just in case devs want some visibility here.
| // Types appear to be getting confused in env setup, using the overloaded NodeJS Blob definition, which uses NodeJS.ReadableStream instead | ||
| // of the DOM type definitions. For definitions, See consumers.d.ts vs. lib.dom.d.ts. (CJP) | ||
| this.chunkedStreamIterable = new ChunkedStreamIterable( | ||
| this.chunkedIterable = new ChunkedStreamIterable( |
There was a problem hiding this comment.
went ahead and renamed these guys to be more generic, given the potential fallback.
| @@ -268,6 +366,8 @@ export class UpChunk { | |||
| private eventTarget: EventTarget<Record<EventName, UpchunkEvent>>; | |||
|
|
|||
| constructor(options: UpChunkOptions) { | |||
| this.eventTarget = new EventTarget(); | |||
There was a problem hiding this comment.
moved this up top so it's ready to go for any potential code, including any refactors to constructor setup
| this.dispatch('error', { | ||
| message: `Unable to read file of size ${this.file.size} bytes. Try loading from another browser.`, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
make sure we bail to not get unexpected success states and the like
src/upchunk.ts
Outdated
| @@ -290,20 +390,46 @@ export class UpChunk { | |||
| this.success = false; | |||
| this.nextChunkRangeStart = 0; | |||
|
|
|||
| if (options.useFileSliceFallback) { | |||
There was a problem hiding this comment.
Explicitly calling it a workaround may help us/contributors remember why it's like this. How would you feel about useLargeFileWorkaround or something?
| @@ -150,6 +158,95 @@ export class ChunkedStreamIterable implements AsyncIterable<Blob> { | |||
| } | |||
| } | |||
|
|
|||
| export class ChunkedFileIterable implements ChunkedIterable { | |||
There was a problem hiding this comment.
Said this in Slack, but I'm personally fine with this explicitly being a band-aid. We want to delete this code if Safari ever patches the issue, so I don't see any reason to spend more time than we have to on the fallback.
|
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. |
|
Went ahead and spruced up some other docs bits and bobs that I noticed. |
This PR adds an "opt in" (via config options) fallback for cases where the (newer)
ReadableStreamFile/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
useFileSliceFallbackoptions flag totrue.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'sReadableStream. However, we migrated to usingReadableStreaminstead 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).