diff --git a/packages/dropzone/CHANGELOG.md b/packages/dropzone/CHANGELOG.md index 7b754ea09..aca6e2051 100644 --- a/packages/dropzone/CHANGELOG.md +++ b/packages/dropzone/CHANGELOG.md @@ -1,3 +1,27 @@ +## 6.3.2 + +### Patch Changes + +- [#2371](https://github.com/enyo/dropzone/pull/2371) [`536d94a`](https://github.com/enyo/dropzone/commit/536d94afd9da8f55b4e4976bd527909946e75e4a) - Fix `cancelUpload` leaving parallel chunks uploading. + + `file.xhr` only holds the request that started last, so cancelling a chunked upload running with `parallelChunkUploads` aborted that one request and left every other in-flight chunk streaming to the server — burning the user's bandwidth and writing orphaned chunks for a file the UI already showed as canceled. + + Every chunk keeps its own request, so `cancelUpload` now aborts all of the ones still running. Uploads that are not chunked are unaffected. + +- [#2372](https://github.com/enyo/dropzone/pull/2372) [`a1a67df`](https://github.com/enyo/dropzone/commit/a1a67dfe16c100b2346974c6d3a7270fc661c423) - Fix `emit` skipping a listener when another one removes itself. + + `emit` walked the live callback array, so a listener that called `off` for itself — the usual shape of a one-shot listener, and of teardown code — spliced the array out from under the loop and the listener registered right after it never ran. `emit` now iterates over a snapshot. + + One consequence worth knowing about: a listener registered from inside another listener no longer runs during that same `emit`, it runs from the next one. That is what Node's `EventEmitter` does, and it is the only way to keep the removal case correct. + +- [#2370](https://github.com/enyo/dropzone/pull/2370) [`0e3625d`](https://github.com/enyo/dropzone/commit/0e3625d05d5861561447be3de64e5481e2186bb1) - Fix the thumbnail queue deadlocking when a file cannot be read. + + `createThumbnail` only listened for `FileReader`'s `load` event. A file that had been moved, locked by another process, or was otherwise unreadable since it was dropped fires `error` instead, so the callback was never invoked and `_processThumbnailQueue` kept its lock forever: no file added afterwards got a thumbnail, and with `resizeWidth`/`resizeHeight` or a `transformFile` that uses `createThumbnail`, the upload never started either. + + The read error now reaches the callback the same way an undecodable image already did, so the file gets `dictThumbnailError` and the queue moves on. + + `DropzoneThumbnailCallback` says what it has always done, too: its first argument is `string | Event`, the error event standing in for the data URL when no thumbnail could be produced. That also fixes `displayExistingFile`, which used to emit that event as a thumbnail when the image URL failed to load, leaving the preview with `img.src` set to `"[object Event]"`. + ## 6.3.1 ### Patch Changes diff --git a/packages/dropzone/package.json b/packages/dropzone/package.json index f7e42efcc..2926f97a0 100644 --- a/packages/dropzone/package.json +++ b/packages/dropzone/package.json @@ -1,6 +1,6 @@ { "name": "dropzone", - "version": "6.3.1", + "version": "6.3.2", "description": "Handles drag and drop of files for you.", "keywords": [ "drag and drop", diff --git a/packages/dropzone/src/dropzone.ts b/packages/dropzone/src/dropzone.ts index a4e584576..89f8a13aa 100644 --- a/packages/dropzone/src/dropzone.ts +++ b/packages/dropzone/src/dropzone.ts @@ -68,9 +68,14 @@ export type DropzoneTransformCallback = (file: DropzoneFile | Blob) => void; /** * Invoked with the rendered thumbnail as a data URL, and the canvas it was * drawn on -- which is null when the image needed no resizing. + * + * There is no separate error callback: when the thumbnail cannot be produced, + * because the file cannot be read or the image cannot be decoded, this is + * invoked with the error event in place of the data URL. Check with + * `typeof dataUrl === "string"` before using it. */ export type DropzoneThumbnailCallback = ( - dataUrl: string, + dataUrl: string | Event, canvas?: HTMLCanvasElement | null, ) => void; @@ -979,11 +984,11 @@ export default class Dropzone extends Emitter { this.options.thumbnailHeight, this.options.thumbnailMethod, true, - (dataUrl: string) => { - // `createThumbnailFromUrl` hands its callback the error event when the - // image cannot be decoded, so anything that is not a data URL means - // the thumbnail failed. Emitting it as one would set the preview's - // `img.src` to "[object Event]" and render a broken image. See #2218. + (dataUrl: string | Event) => { + // Both failure paths -- the file not being readable, and the image + // not being decodable -- hand the callback the error event. Emitting + // that as a thumbnail would set the preview's `img.src` to + // "[object Event]" and render a broken image. See #2218 and #2365. if (typeof dataUrl === "string") { this.emit("thumbnail", file, dataUrl); } else { @@ -1039,9 +1044,10 @@ export default class Dropzone extends Emitter { height, resizeMethod, true, - (dataUrl: string, canvas?: HTMLCanvasElement | null) => { + (dataUrl: string | Event, canvas?: HTMLCanvasElement | null) => { if (canvas == null) { - // The image has not been resized + // The image has not been resized, or could not be read or decoded at + // all -- either way there is nothing to send but the original file. return callback(file); } else { let { resizeMimeType } = this.options; @@ -1094,6 +1100,18 @@ export default class Dropzone extends Emitter { this.createThumbnailFromUrl(file, width, height, resizeMethod, fixOrientation, callback); }; + // A file that cannot be read -- moved, locked by another process, or on a + // drive that went away since it was dropped -- fires `error` and never + // `load`. Without this the callback is never invoked at all, which leaves + // `_processThumbnailQueue` holding its lock forever: no later file gets a + // thumbnail, and an upload waiting on `transformFile` never sends. See + // #2365. + fileReader.onerror = (e) => { + if (callback != null) { + callback(e); + } + }; + fileReader.readAsDataURL(file); } @@ -1117,8 +1135,14 @@ export default class Dropzone extends Emitter { this.emit("thumbnail", mockFile, imageUrl); if (callback) callback(); } else { - let onDone = (thumbnail: string) => { - this.emit("thumbnail", mockFile, thumbnail); + let onDone = (thumbnail: string | Event) => { + // An image URL that does not load -- gone from the server, or blocked + // by CORS -- arrives here as the error event. Emitting that as a + // thumbnail would set the preview's `img.src` to "[object Event]", so + // leave the preview alone and just report that we are done. + if (typeof thumbnail === "string") { + this.emit("thumbnail", mockFile, thumbnail); + } if (callback) callback(); }; mockFile.dataURL = imageUrl; @@ -1247,9 +1271,8 @@ export default class Dropzone extends Emitter { if (callback != null) { // The same callback does double duty: it receives the thumbnail on - // success, and is called as the image's error handler on failure, where - // an Event arrives instead of a data URL. - img.onerror = callback as unknown as OnErrorEventHandler; + // success, and the error event on failure. + img.onerror = (e) => callback(e); } return (img.src = file.dataURL!); @@ -1321,7 +1344,18 @@ export default class Dropzone extends Emitter { for (let groupedFile of groupedFiles) { groupedFile.status = Dropzone.CANCELED; } - if (typeof file.xhr !== "undefined") { + if (file.upload.chunked && file.upload.chunks) { + // `file.xhr` only ever holds the request that started last, so with + // `parallelChunkUploads` aborting it leaves every other chunk + // streaming to the server for a file the user has already canceled. + // Each chunk keeps its own request, so abort them all. See #2366. + for (let chunk of file.upload.chunks) { + if (chunk && chunk.xhr && chunk.status === Dropzone.UPLOADING) { + chunk.status = Dropzone.CANCELED; + chunk.xhr.abort(); + } + } + } else if (typeof file.xhr !== "undefined") { file.xhr.abort(); } for (let groupedFile of groupedFiles) { diff --git a/packages/dropzone/src/emitter.ts b/packages/dropzone/src/emitter.ts index a4a201f5f..189a7b986 100644 --- a/packages/dropzone/src/emitter.ts +++ b/packages/dropzone/src/emitter.ts @@ -25,7 +25,12 @@ export default class Emitter { let callbacks = this._callbacks[event!]; if (callbacks) { - for (let callback of callbacks) { + // Iterating the live array would skip a listener whenever one of them + // removes itself: `off` splices in place, so everything after the + // removed entry shifts down past the loop's index. Snapshotting also + // means a listener added by another listener does not run until the + // next emit, which is how EventEmitter behaves. See #2367. + for (let callback of callbacks.slice()) { callback.apply(this, args); } } diff --git a/packages/dropzone/test/unit-tests/all.js b/packages/dropzone/test/unit-tests/all.js index 3a71785c8..c2220ab09 100644 --- a/packages/dropzone/test/unit-tests/all.js +++ b/packages/dropzone/test/unit-tests/all.js @@ -1363,6 +1363,52 @@ describe("Dropzone", function () { dropzone.addFile(corrupt); })); + it("should emit an error and keep the queue moving if a file can't be read", async function () { + dropzone.processFile = function () {}; + dropzone.uploadFile = function () {}; + + // A file that has gone away since it was dropped, or is locked by + // another process, makes FileReader fire `error` and never `load`. + // See #2365. + let readAsDataURL = vi + .spyOn(FileReader.prototype, "readAsDataURL") + .mockImplementation(function () { + setTimeout(() => this.dispatchEvent(new ProgressEvent("error")), 0); + }); + + let unreadable = getMockFile("image/png", "unreadable.png"); + + let message = await new Promise(function (resolve) { + dropzone.on("error", function (file, message) { + if (file === unreadable) resolve(message); + }); + dropzone.addFile(unreadable); + }); + + expect(message).toBe(dropzone.options.dictThumbnailError); + // The lock has to be released, or nothing queued behind the failed + // file is ever processed again. + expect(dropzone._processingThumbnail).toBe(false); + + readAsDataURL.mockRestore(); + + let readable = await new Promise(function (resolve) { + let canvas = document.createElement("canvas"); + canvas.width = canvas.height = 10; + canvas.toBlob( + (blob) => resolve(new File([blob], "readable.png", { type: "image/png" })), + "image/png", + ); + }); + + let thumbnailed = await new Promise(function (resolve) { + dropzone.on("thumbnail", (file) => resolve(file)); + dropzone.addFile(readable); + }); + + expect(thumbnailed).toBe(readable); + }); + it("should not let the thumbnail itself be dragged", function () { dropzone.processFile = function () {}; dropzone.uploadFile = function () {}; @@ -2162,6 +2208,36 @@ describe("Dropzone", function () { }, 10); })); + it("should abort every chunk still in flight when the upload is canceled", () => + new Promise((done) => { + dropzone.options.chunking = true; + dropzone.options.chunkSize = 1; + dropzone.options.parallelChunkUploads = 3; + + let file = getMockFile("text/html", "chunked-file", ["abcdef"]); + dropzone.addFile(file); + + setTimeout(function () { + // Three of the six chunks are in flight, each with its own + // request. + expect(requests.length).toBe(3); + expect(requests.map((request) => request.aborted)).toEqual([false, false, false]); + + dropzone.cancelUpload(file); + + expect(file.status).toBe(Dropzone.CANCELED); + // `file.xhr` only holds the request that started last, so the + // other two used to keep streaming to the server. See #2366. + expect(requests.map((request) => request.aborted)).toEqual([true, true, true]); + expect(file.upload.chunks.map((chunk) => chunk.status)).toEqual([ + Dropzone.CANCELED, + Dropzone.CANCELED, + Dropzone.CANCELED, + ]); + done(); + }, 10); + })); + it("should never start fewer than one chunk", () => new Promise((done) => { startChunked({ parallelChunkUploads: 0 }); diff --git a/packages/dropzone/test/unit-tests/emitter.js b/packages/dropzone/test/unit-tests/emitter.js index d2fcb2d93..ee1946209 100644 --- a/packages/dropzone/test/unit-tests/emitter.js +++ b/packages/dropzone/test/unit-tests/emitter.js @@ -70,6 +70,63 @@ describe("Emitter", function () { return expect(callCount2).toBe(1); }); + describe(".emit() while the listeners change", function () { + it("should still run every listener when one removes itself", function () { + let calls = []; + + // A one-shot listener is the usual way to end up here: it unregisters + // itself from inside the very emit that is walking the array. Splicing + // it out shifted everything after it down past the loop's index, so the + // listener that followed was silently skipped. See #2367. + let first = function () { + calls.push("first"); + emitter.off("test", first); + }; + emitter.on("test", first); + emitter.on("test", () => calls.push("second")); + emitter.on("test", () => calls.push("third")); + + emitter.emit("test"); + + expect(calls).toEqual(["first", "second", "third"]); + + // And it really is gone for the next one. + calls = []; + emitter.emit("test"); + expect(calls).toEqual(["second", "third"]); + }); + + it("should still run the listeners that were registered when off() removes them all", function () { + let calls = []; + + emitter.on("test", function () { + calls.push("first"); + emitter.off("test"); + }); + emitter.on("test", () => calls.push("second")); + + emitter.emit("test"); + + expect(calls).toEqual(["first", "second"]); + expect(emitter._callbacks["test"]).toBe(undefined); + }); + + it("should not run a listener added by another listener until the next emit", function () { + let calls = []; + + emitter.on("test", function () { + calls.push("first"); + emitter.on("test", () => calls.push("added")); + }); + + emitter.emit("test"); + expect(calls).toEqual(["first"]); + + emitter.emit("test"); + expect(calls).toEqual(["first", "first", "added"]); + }); + }); + describe(".off()", function () { let callback1 = function () {}; let callback2 = function () {}; diff --git a/packages/dropzone/test/unit-tests/public-api.js b/packages/dropzone/test/unit-tests/public-api.js index a643a949d..8112086ec 100644 --- a/packages/dropzone/test/unit-tests/public-api.js +++ b/packages/dropzone/test/unit-tests/public-api.js @@ -66,6 +66,20 @@ describe("public API", function () { expect(dropzone.createThumbnailFromUrl).toHaveBeenCalledTimes(1); }); + it("should not emit a thumbnail when the image url fails to load", async function () { + create(); + let thumbnail = null; + dropzone.on("thumbnail", (file, url) => (thumbnail = url)); + + // A URL that never resolves to an image: the preview would otherwise be + // handed the error event and set `img.src` to "[object Event]". + await new Promise((done) => + dropzone.displayExistingFile(mockFile(), "/does-not-exist.png", done), + ); + + expect(thumbnail).toBe(null); + }); + // #2003 and the 7.0 roadmap: files added this way never reach this.files, // so maxFiles cannot see them. The documented workaround is to push them // by hand, which is why fixing it is a breaking change rather than a