From 6108658f7b845137b8f3918080dfce99a83a4ff9 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 8 Jun 2023 15:11:28 +0100 Subject: [PATCH] fix(remix): Extract deferred responses correctly in root loaders. --- packages/remix/src/utils/instrumentServer.ts | 9 +++++- packages/remix/src/utils/types.ts | 9 ++++++ packages/remix/src/utils/vendor/response.ts | 17 ++++++++++- packages/remix/test/integration/app/root.tsx | 4 ++- .../routes/loader-defer-response/index.tsx | 20 +++++++++++++ .../test/client/root-loader.test.ts | 16 +++++++++++ .../integration/test/server/loader.test.ts | 28 +++++++++++++++++++ 7 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 packages/remix/test/integration/app/routes/loader-defer-response/index.tsx diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 7fbff4bb6bd8..ef5449067df9 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -27,7 +27,7 @@ import type { ServerRoute, ServerRouteManifest, } from './types'; -import { extractData, getRequestMatch, isResponse, json, matchServerRoutes } from './vendor/response'; +import { extractData, getRequestMatch, isDeferredData, isResponse, json, matchServerRoutes } from './vendor/response'; import { normalizeRemixRequest } from './web-fetch'; // Flag to track if the core request handler is instrumented. @@ -229,6 +229,13 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { const res = await origLoader.call(this, args); const traceAndBaggage = getTraceAndBaggage(); + if (isDeferredData(res)) { + return { + ...res.data, + ...traceAndBaggage, + }; + } + // Note: `redirect` and `catch` responses do not have bodies to extract if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) { const data = await extractData(res); diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/types.ts index 642f6eef76cb..74dcf10215cc 100644 --- a/packages/remix/src/utils/types.ts +++ b/packages/remix/src/utils/types.ts @@ -62,6 +62,15 @@ export interface RouteData { [routeId: string]: AppData; } +export type DeferredData = { + data: Record; + init?: ResponseInit; + deferredKeys: string[]; + subscribe(fn: (aborted: boolean, settledKey?: string) => void): () => boolean; + cancel(): void; + resolveData(signal: AbortSignal): Promise; +}; + export interface MetaFunction { (args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor; } diff --git a/packages/remix/src/utils/vendor/response.ts b/packages/remix/src/utils/vendor/response.ts index ab5e02425c8e..ae85fff74734 100644 --- a/packages/remix/src/utils/vendor/response.ts +++ b/packages/remix/src/utils/vendor/response.ts @@ -6,7 +6,7 @@ // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. -import type { ReactRouterDomPkg, RouteMatch, ServerRoute } from '../types'; +import type { DeferredData, ReactRouterDomPkg, RouteMatch, ServerRoute } from '../types'; /** * Based on Remix Implementation @@ -124,3 +124,18 @@ export function getRequestMatch(url: URL, matches: RouteMatch[]): R return match; } + +/** + * https://github.com/remix-run/remix/blob/3e589152bc717d04e2054c31bea5a1056080d4b9/packages/remix-server-runtime/responses.ts#L75-L85 + */ +export function isDeferredData(value: any): value is DeferredData { + const deferred: DeferredData = value; + return ( + deferred && + typeof deferred === 'object' && + typeof deferred.data === 'object' && + typeof deferred.subscribe === 'function' && + typeof deferred.cancel === 'function' && + typeof deferred.resolveData === 'function' + ); +} diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app/root.tsx index e7ec56171904..1e716237343c 100644 --- a/packages/remix/test/integration/app/root.tsx +++ b/packages/remix/test/integration/app/root.tsx @@ -1,4 +1,4 @@ -import { MetaFunction, LoaderFunction, json, redirect } from '@remix-run/node'; +import { MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; import { withSentry } from '@sentry/remix'; @@ -24,6 +24,8 @@ export const loader: LoaderFunction = async ({ request }) => { }; case 'json': return json({ data_one: [], data_two: 'a string' }, { headers: { 'Cache-Control': 'max-age=300' } }); + case 'defer': + return defer({ data_one: [], data_two: 'a string' }); case 'null': return null; case 'undefined': diff --git a/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx b/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx new file mode 100644 index 000000000000..b55d8dfede47 --- /dev/null +++ b/packages/remix/test/integration/app/routes/loader-defer-response/index.tsx @@ -0,0 +1,20 @@ +import { defer, LoaderFunction } from '@remix-run/node'; +import { useLoaderData } from '@remix-run/react'; + +type LoaderData = { id: string }; + +export const loader: LoaderFunction = async ({ params: { id } }) => { + return defer({ + id, + }); +}; + +export default function LoaderJSONResponse() { + const data = useLoaderData(); + + return ( +
+

{data && data.id ? data.id : 'Not Found'}

+
+ ); +} diff --git a/packages/remix/test/integration/test/client/root-loader.test.ts b/packages/remix/test/integration/test/client/root-loader.test.ts index 774cbaa3e4c0..1a68cdeb9d4a 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -72,6 +72,22 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a `J }); }); +test('should inject `sentry-trace` and `baggage` into root loader returning a deferred response', async ({ page }) => { + await page.goto('/?type=defer'); + + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); + + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, + }); +}); + test('should inject `sentry-trace` and `baggage` into root loader returning `null`.', async ({ page }) => { await page.goto('/?type=null'); diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 907875949f30..2545f63d6e92 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -187,4 +187,32 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }); }); + + it('correctly instruments a deferred loader', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/loader-defer-response`; + const envelope = await env.getEnvelopeRequest({ url, envelopeType: 'transaction' }); + const transaction = envelope[2]; + + assertSentryTransaction(transaction, { + transaction: 'root', + transaction_info: { + source: 'route', + }, + spans: [ + { + description: 'root', + op: 'function.remix.loader', + }, + { + description: 'routes/loader-defer-response/index', + op: 'function.remix.loader', + }, + { + description: 'root', + op: 'function.remix.document_request', + }, + ], + }); + }); });