From 87829b752a9f9a3688f2e47b8ac9460c495f6dc4 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 11 Jul 2023 13:56:56 -0400 Subject: [PATCH 1/4] Better handling of deferred promises that resolve/reject with undefined --- .changeset/defer-resolve-undefined.md | 5 +++++ packages/router/utils.ts | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 .changeset/defer-resolve-undefined.md diff --git a/.changeset/defer-resolve-undefined.md b/.changeset/defer-resolve-undefined.md new file mode 100644 index 0000000000..2af79ce785 --- /dev/null +++ b/.changeset/defer-resolve-undefined.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Better handling of deferred promises that resolve/reject with `undefined` diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 14b4126d6a..7d96f337a1 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1357,6 +1357,30 @@ export class DeferredData { return Promise.reject(error); } + // TODO: We oughta just do the same for `error`. If so we should add an + // `isError` boolean to this function so we stop using the presence of + // error/data as the switch + + // Option 1 + if (data === undefined) { + Object.defineProperty(promise, "_error", { + get: () => + new Error( + `Deferred data for key ${key} resolved to \`undefined\`, you must resolve with a value or \`null\`.` + ), + }); + this.emit(false, key); + return Promise.reject(error); + } + + // Option 2 + if (data === undefined) { + console.warn( + `Deferred data for key ${key} resolved to \`undefined\`, defaulting to \`null\`.` + ); + data = null; + } + Object.defineProperty(promise, "_data", { get: () => data }); this.emit(false, key); return data; From 3437ce662009ae83c1ebe677a9f485c2a3e48289 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Tue, 11 Jul 2023 11:51:05 -0700 Subject: [PATCH 2/4] feat: reject on undefined promise resolution --- packages/router/__tests__/router-test.ts | 64 ++++++++++++++++++++++++ packages/router/utils.ts | 18 ++----- 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 23d2e6b072..da14933562 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -14310,6 +14310,12 @@ describe("a router", () => { ), }); } + if (new URL(request.url).searchParams.has("undefined")) { + return defer({ + critical: "loader", + lazy: new Promise((r) => setTimeout(() => r(undefined), 10)), + }); + } if (new URL(request.url).searchParams.has("status")) { return defer( { @@ -15114,6 +15120,42 @@ describe("a router", () => { }); }); + it("should return rejected DeferredData on symbol for resolved undefined", async () => { + let context = (await query( + createRequest("/parent/deferred?undefined") + )) as StaticHandlerContext; + expect(context).toMatchObject({ + loaderData: { + parent: "PARENT LOADER", + deferred: { + critical: "loader", + lazy: expect.trackedPromise(), + }, + }, + activeDeferreds: { + deferred: expect.deferredData(false), + }, + }); + await new Promise((r) => setTimeout(r, 10)); + expect(context).toMatchObject({ + loaderData: { + parent: "PARENT LOADER", + deferred: { + critical: "loader", + lazy: expect.trackedPromise( + undefined, + new Error( + `Deferred data for key lazy resolved to \`undefined\`, you must resolve with a value or \`null\`.` + ) + ), + }, + }, + activeDeferreds: { + deferred: expect.deferredData(true), + }, + }); + }); + it("should return DeferredData on symbol with status + headers", async () => { let context = (await query( createRequest("/parent/deferred?status") @@ -16179,6 +16221,28 @@ describe("a router", () => { expect(result[UNSAFE_DEFERRED_SYMBOL]).deferredData(true); }); + it("should return rejected DeferredData on symbol for resolved undefined", async () => { + let result = await queryRoute( + createRequest("/parent/deferred?undefined") + ); + expect(result).toMatchObject({ + critical: "loader", + lazy: expect.trackedPromise(), + }); + expect(result[UNSAFE_DEFERRED_SYMBOL]).deferredData(false); + await new Promise((r) => setTimeout(r, 10)); + expect(result).toMatchObject({ + critical: "loader", + lazy: expect.trackedPromise( + null, + new Error( + `Deferred data for key lazy resolved to \`undefined\`, you must resolve with a value or \`null\`.` + ) + ), + }); + expect(result[UNSAFE_DEFERRED_SYMBOL]).deferredData(true); + }); + it("should return DeferredData on symbol with status + headers", async () => { let result = await queryRoute( createRequest("/parent/deferred?status") diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 7d96f337a1..e65d98b692 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1351,17 +1351,15 @@ export class DeferredData { this.unlistenAbortSignal(); } - if (error) { + // use argument length to detect error or value resolution + if (arguments.length === 3) { Object.defineProperty(promise, "_error", { get: () => error }); this.emit(false, key); return Promise.reject(error); } - // TODO: We oughta just do the same for `error`. If so we should add an - // `isError` boolean to this function so we stop using the presence of - // error/data as the switch - - // Option 1 + // If the promise was resolved with undefined, we'll throw an error as you + // should always resolve with a value or null if (data === undefined) { Object.defineProperty(promise, "_error", { get: () => @@ -1373,14 +1371,6 @@ export class DeferredData { return Promise.reject(error); } - // Option 2 - if (data === undefined) { - console.warn( - `Deferred data for key ${key} resolved to \`undefined\`, defaulting to \`null\`.` - ); - data = null; - } - Object.defineProperty(promise, "_data", { get: () => data }); this.emit(false, key); return data; From e52324a060eb2acb7f1684335633978f41f912dd Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Tue, 11 Jul 2023 12:42:40 -0700 Subject: [PATCH 3/4] update to handle error and data at once --- packages/router/__tests__/router-test.ts | 6 +++--- packages/router/utils.ts | 24 +++++++++++------------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index da14933562..f298b937f7 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -15143,9 +15143,9 @@ describe("a router", () => { deferred: { critical: "loader", lazy: expect.trackedPromise( - undefined, + null, new Error( - `Deferred data for key lazy resolved to \`undefined\`, you must resolve with a value or \`null\`.` + `Deferred data for key "lazy" resolved/rejected with \`undefined\`, you must resolve/reject with a value or \`null\`.` ) ), }, @@ -16236,7 +16236,7 @@ describe("a router", () => { lazy: expect.trackedPromise( null, new Error( - `Deferred data for key lazy resolved to \`undefined\`, you must resolve with a value or \`null\`.` + `Deferred data for key "lazy" resolved/rejected with \`undefined\`, you must resolve/reject with a value or \`null\`.` ) ), }); diff --git a/packages/router/utils.ts b/packages/router/utils.ts index e65d98b692..9830d3078d 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1317,7 +1317,7 @@ export class DeferredData { // We store a little wrapper promise that will be extended with // _data/_error props upon resolve/reject let promise: TrackedPromise = Promise.race([value, this.abortPromise]).then( - (data) => this.onSettle(promise, key, null, data as unknown), + (data) => this.onSettle(promise, key, undefined, data as unknown), (error) => this.onSettle(promise, key, error as unknown) ); @@ -1351,22 +1351,20 @@ export class DeferredData { this.unlistenAbortSignal(); } - // use argument length to detect error or value resolution - if (arguments.length === 3) { - Object.defineProperty(promise, "_error", { get: () => error }); + // If the promise was resolved/rejected with undefined, we'll throw an error as you + // should always resolve with a value or null + if (error === undefined && data === undefined) { + let undefinedError = new Error( + `Deferred data for key "${key}" resolved/rejected with \`undefined\`, ` + + `you must resolve/reject with a value or \`null\`.` + ); + Object.defineProperty(promise, "_error", { get: () => undefinedError }); this.emit(false, key); - return Promise.reject(error); + return Promise.reject(undefinedError); } - // If the promise was resolved with undefined, we'll throw an error as you - // should always resolve with a value or null if (data === undefined) { - Object.defineProperty(promise, "_error", { - get: () => - new Error( - `Deferred data for key ${key} resolved to \`undefined\`, you must resolve with a value or \`null\`.` - ), - }); + Object.defineProperty(promise, "_error", { get: () => error }); this.emit(false, key); return Promise.reject(error); } From ba5804c4ac680cacc6b65c7a6a985158ec6a4e67 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 11 Jul 2023 16:15:56 -0400 Subject: [PATCH 4/4] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 083bcd3098..5fe35993c3 100644 --- a/package.json +++ b/package.json @@ -109,7 +109,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "46.9 kB" + "none": "47.2 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "13.8 kB"