From f4032f6fdd2e78aee9581caadaae20969888ae8f Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 14:44:35 +0100 Subject: [PATCH 1/7] fix(remix): Support merging `json` responses from root loader functions. --- packages/remix/package.json | 3 +- packages/remix/src/utils/instrumentServer.ts | 12 +- packages/remix/test/integration/app/root.tsx | 31 ++- .../test/client/root-loader.test.ts | 187 ++++++++++++++++++ 4 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 packages/remix/test/integration/test/client/root-loader.test.ts diff --git a/packages/remix/package.json b/packages/remix/package.json index b01969347ea8..ea314a25ac5d 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -61,8 +61,9 @@ "lint:prettier": "prettier --check \"{src,test,scripts}/**/*.ts\"", "test": "run-s test:unit", "test:integration": "run-s test:integration:prepare test:integration:client test:integration:server", + "test:integration:clean": "(cd test/integration && rimraf .cache build node_modules)", "test:integration:ci": "run-s test:integration:prepare test:integration:client:ci test:integration:server", - "test:integration:prepare": "(cd test/integration && yarn)", + "test:integration:prepare": "yarn test:integration:clean && (cd test/integration && yarn)", "test:integration:client": "yarn playwright install-deps && yarn playwright test test/integration/test/client/", "test:integration:client:ci": "yarn test:integration:client --browser='all' --reporter='line'", "test:integration:server": "jest --config=test/integration/jest.config.js test/integration/test/server/", diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 511ca27b6e01..3f5bfd4ff4a2 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -262,8 +262,18 @@ function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { return async function (this: unknown, args: DataFunctionArgs): Promise { const res = await origLoader.call(this, args); + const traceAndBaggage = getTraceAndBaggage(); - return { ...res, ...getTraceAndBaggage() }; + if (isResponse(res)) { + const resClone = res.clone(); + const data = await extractData(resClone); + + if (typeof data === 'object') { + return { ...data, ...traceAndBaggage }; + } + } + + return { ...res, ...traceAndBaggage }; }; } diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app/root.tsx index e2833effa83b..0c7b3f2f2b9c 100644 --- a/packages/remix/test/integration/app/root.tsx +++ b/packages/remix/test/integration/app/root.tsx @@ -1,4 +1,4 @@ -import type { MetaFunction } from '@remix-run/node'; +import { MetaFunction, LoaderFunction, json, redirect } from '@remix-run/node'; import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; import { withSentry } from '@sentry/remix'; @@ -10,7 +10,34 @@ export const meta: MetaFunction = ({ data }) => ({ baggage: data.sentryBaggage, }); -export function App() { +export const loader: LoaderFunction = async ({ request }) => { + const url = new URL(request.url); + const type = url.searchParams.get('type'); + + switch (type) { + case 'empty': + return {}; + case 'plain': + return { + data_one: [], + data_two: 'a string', + }; + case 'json': + return json({ data_one: [], data_two: 'a string' }, { headers: { 'Cache-Control': 'max-age=300' } }); + case 'null': + return null; + case 'undefined': + return undefined; + case 'throw-redirect': + throw redirect('/plain'); + case 'return-redirect': + return redirect('/plain'); + case 'return-redirect-json': + return redirect('/json'); + } +}; + +function App() { return ( diff --git a/packages/remix/test/integration/test/client/root-loader.test.ts b/packages/remix/test/integration/test/client/root-loader.test.ts new file mode 100644 index 000000000000..5c4699658ca4 --- /dev/null +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -0,0 +1,187 @@ +import { test, expect, Page } from '@playwright/test'; + +export function getRouteData(page: Page): Promise { + return page.evaluate('window.__remixContext.routeData').catch(err => { + return {}; + }); +} + +test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => { + await page.goto('/?type=empty'); + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning a plain object.', async ({ page }) => { + await page.goto('/?type=plain'); + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + data_one: [], + data_two: 'a string', + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning a `JSON response`.', async ({ page }) => { + await page.goto('/?type=json'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + data_one: [], + data_two: 'a string', + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning `null`.', async ({ page }) => { + await page.goto('/?type=null'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning `undefined`.', async ({ page }) => { + await page.goto('/?type=undefined'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + debugger; + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader throwing a redirection to a plain object.', async ({ + page, +}) => { + await page.goto('/?type="throw-redirect"'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + debugger; + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to a plain object', async ({ + page, +}) => { + await page.goto('/?type="return-redirect"'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + debugger; + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); + +test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to a `JSON response`', async ({ + page, +}) => { + await page.goto('/?type="return-redirect"'); + + const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); + const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + + expect(sentryTraceContent).toEqual(expect.any(String)); + + const sentryBaggageTag = await page.$('meta[name="baggage"]'); + const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + + expect(sentryBaggageContent).toEqual(expect.any(String)); + + const rootData = (await getRouteData(page))['root']; + + debugger; + expect(rootData).toMatchObject({ + sentryTrace: sentryTraceContent, + sentryBaggage: sentryBaggageContent, + }); +}); From 80d1711b0e1f4fb3c506ecfa382684542aebd3ec Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 16:16:25 +0100 Subject: [PATCH 2/7] Remove debuggers --- .../remix/test/integration/test/client/root-loader.test.ts | 4 ---- 1 file changed, 4 deletions(-) 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 5c4699658ca4..970a431e7d91 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -107,7 +107,6 @@ test('should inject `sentry-trace` and `baggage` into root loader returning `und const rootData = (await getRouteData(page))['root']; - debugger; expect(rootData).toMatchObject({ sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent, @@ -131,7 +130,6 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red const rootData = (await getRouteData(page))['root']; - debugger; expect(rootData).toMatchObject({ sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent, @@ -155,7 +153,6 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re const rootData = (await getRouteData(page))['root']; - debugger; expect(rootData).toMatchObject({ sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent, @@ -179,7 +176,6 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re const rootData = (await getRouteData(page))['root']; - debugger; expect(rootData).toMatchObject({ sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent, From 2dff4d66e984346bb782a82f3dc5241169e42ac6 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 17:31:10 +0100 Subject: [PATCH 3/7] Update tests. --- packages/remix/src/utils/instrumentServer.ts | 5 ++-- .../integration/test/server/action.test.ts | 7 ++++++ .../integration/test/server/loader.test.ts | 24 ++++++------------- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 3f5bfd4ff4a2..bfffe77bbf2d 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -207,6 +207,7 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' try { const span = activeTransaction.startChild({ op: `remix.server.${name}`, + // TODO: Consider using the `id` of the route this function belongs to description: activeTransaction.name, tags: { name, @@ -235,8 +236,8 @@ function makeWrappedAction(origAction: DataFunction): DataFunction { return makeWrappedDataFunction(origAction, 'action'); } -function makeWrappedLoader(origAction: DataFunction): DataFunction { - return makeWrappedDataFunction(origAction, 'loader'); +function makeWrappedLoader(origLoader: DataFunction): DataFunction { + return makeWrappedDataFunction(origLoader, 'loader'); } function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } { diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 1c32cd86574d..3a27251e2bdc 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -22,6 +22,13 @@ describe('Remix API Actions', () => { description: 'routes/action-json-response/$id', op: 'remix.server.action', }, + // TODO: These two spans look exactly the same, but they are not. + // One is from the parent route, and the other is from the route we are reaching. + // We need to pass the names of the routes as their descriptions while wrapping loaders and actions. + { + description: 'routes/action-json-response/$id', + op: 'remix.server.loader', + }, { description: 'routes/action-json-response/$id', op: 'remix.server.loader', diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 4da695979b95..2d79a0601333 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -9,23 +9,6 @@ import { jest.spyOn(console, 'error').mockImplementation(); describe('Remix API Loaders', () => { - it('does not add a loader if there is not one defined.', async () => { - const baseURL = await runServer(); - const url = `${baseURL}/`; - const envelope = await getEnvelopeRequest(url); - const transaction = envelope[2]; - - assertSentryTransaction(transaction, { - transaction: 'root', - spans: [ - { - description: 'root', - op: 'remix.server.documentRequest', - }, - ], - }); - }); - it('reports an error thrown from the loader', async () => { const baseURL = await runServer(); const url = `${baseURL}/loader-json-response/-2`; @@ -75,6 +58,13 @@ describe('Remix API Loaders', () => { source: 'route', }, spans: [ + // TODO: These two spans look exactly the same, but they are not. + // One is from the parent route, and the other is from the route we are reaching. + // We need to pass the names of the routes as their descriptions while wrapping loaders and actions. + { + description: 'routes/loader-json-response/$id', + op: 'remix.server.loader', + }, { description: 'routes/loader-json-response/$id', op: 'remix.server.loader', From 63419d2da2e3bcb4fef403ae3359bced11ae7ae1 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 17:39:14 +0100 Subject: [PATCH 4/7] Move meta extraction logic into its own utility. --- .../test/client/root-loader.test.ts | 130 +++++++----------- 1 file changed, 53 insertions(+), 77 deletions(-) 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 970a431e7d91..2018aea2ce18 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -1,115 +1,106 @@ import { test, expect, Page } from '@playwright/test'; -export function getRouteData(page: Page): Promise { +function getRouteData(page: Page): Promise { return page.evaluate('window.__remixContext.routeData').catch(err => { + console.warn(err); + return {}; }); } -test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => { - await page.goto('/?type=empty'); +async function extractTraceAndBaggageFromMeta( + page: Page, +): Promise<{ sentryTrace?: string | null; sentryBaggage?: string | null }> { const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - expect(sentryTraceContent).toEqual(expect.any(String)); - const sentryBaggageTag = await page.$('meta[name="baggage"]'); const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); - expect(sentryBaggageContent).toEqual(expect.any(String)); + return { sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent }; +} + +test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => { + await page.goto('/?type=empty'); + + 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: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace, + sentryBaggage, }); }); test('should inject `sentry-trace` and `baggage` into root loader returning a plain object.', async ({ page }) => { await page.goto('/?type=plain'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ data_one: [], data_two: 'a string', - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); test('should inject `sentry-trace` and `baggage` into root loader returning a `JSON response`.', async ({ page }) => { await page.goto('/?type=json'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryTraceContent).toEqual(expect.any(String)); - - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); - - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ data_one: [], data_two: 'a string', - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); test('should inject `sentry-trace` and `baggage` into root loader returning `null`.', async ({ page }) => { await page.goto('/?type=null'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); - - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); test('should inject `sentry-trace` and `baggage` into root loader returning `undefined`.', async ({ page }) => { await page.goto('/?type=undefined'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); - - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); @@ -118,21 +109,16 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red }) => { await page.goto('/?type="throw-redirect"'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); - - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); @@ -141,21 +127,16 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re }) => { await page.goto('/?type="return-redirect"'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); - - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); @@ -164,20 +145,15 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re }) => { await page.goto('/?type="return-redirect"'); - const sentryTraceTag = await page.$('meta[name="sentry-trace"]'); - const sentryTraceContent = await sentryTraceTag?.getAttribute('content'); - - expect(sentryTraceContent).toEqual(expect.any(String)); - - const sentryBaggageTag = await page.$('meta[name="baggage"]'); - const sentryBaggageContent = await sentryBaggageTag?.getAttribute('content'); + const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); - expect(sentryBaggageContent).toEqual(expect.any(String)); + expect(sentryTrace).toEqual(expect.any(String)); + expect(sentryBaggage).toEqual(expect.any(String)); const rootData = (await getRouteData(page))['root']; expect(rootData).toMatchObject({ - sentryTrace: sentryTraceContent, - sentryBaggage: sentryBaggageContent, + sentryTrace: sentryTrace, + sentryBaggage: sentryBaggage, }); }); From 8919b6859636e975cc8cde30d024a919ddc195b6 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 18:51:27 +0100 Subject: [PATCH 5/7] Do not try to extract bodies of `redirect` or `catch` responses. --- packages/remix/src/utils/instrumentServer.ts | 17 +++++++++---- packages/remix/test/integration/app/root.tsx | 13 +++++----- .../test/client/root-loader.test.ts | 24 +++---------------- 3 files changed, 23 insertions(+), 31 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index bfffe77bbf2d..a82b5551f9f5 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -100,7 +100,7 @@ export interface RouteMatch { } // Taken from Remix Implementation -// https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/responses.ts#L54-L62 +// https://github.com/remix-run/remix/blob/32300ec6e6e8025602cea63e17a2201989589eab/packages/remix-server-runtime/responses.ts#L60-L77 // eslint-disable-next-line @typescript-eslint/no-explicit-any function isResponse(value: any): value is Response { return ( @@ -114,6 +114,15 @@ function isResponse(value: any): value is Response { ); } +const redirectStatusCodes = new Set([301, 302, 303, 307, 308]); +function isRedirectResponse(response: Response): boolean { + return redirectStatusCodes.has(response.status); +} + +function isCatchResponse(response: Response): boolean { + return response.headers.get('X-Remix-Catch') != null; +} + // Based on Remix Implementation // https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L131-L145 function extractData(response: Response): Promise { @@ -265,9 +274,9 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { const res = await origLoader.call(this, args); const traceAndBaggage = getTraceAndBaggage(); - if (isResponse(res)) { - const resClone = res.clone(); - const data = await extractData(resClone); + // Note: `redirect` and `catch` responses do not have bodies to extract + if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) { + const data = await extractData(res); if (typeof data === 'object') { return { ...data, ...traceAndBaggage }; diff --git a/packages/remix/test/integration/app/root.tsx b/packages/remix/test/integration/app/root.tsx index 0c7b3f2f2b9c..e7ec56171904 100644 --- a/packages/remix/test/integration/app/root.tsx +++ b/packages/remix/test/integration/app/root.tsx @@ -28,12 +28,13 @@ export const loader: LoaderFunction = async ({ request }) => { return null; case 'undefined': return undefined; - case 'throw-redirect': - throw redirect('/plain'); - case 'return-redirect': - return redirect('/plain'); - case 'return-redirect-json': - return redirect('/json'); + case 'throwRedirect': + throw redirect('/?type=plain'); + case 'returnRedirect': + return redirect('/?type=plain'); + default: { + return {}; + } } }; 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 2018aea2ce18..0a339018eab8 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -1,6 +1,6 @@ import { test, expect, Page } from '@playwright/test'; -function getRouteData(page: Page): Promise { +async function getRouteData(page: Page): Promise { return page.evaluate('window.__remixContext.routeData').catch(err => { console.warn(err); @@ -107,7 +107,7 @@ test('should inject `sentry-trace` and `baggage` into root loader returning `und test('should inject `sentry-trace` and `baggage` into root loader throwing a redirection to a plain object.', async ({ page, }) => { - await page.goto('/?type="throw-redirect"'); + await page.goto('/?type=throwRedirect'); const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); @@ -125,25 +125,7 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to a plain object', async ({ page, }) => { - await page.goto('/?type="return-redirect"'); - - 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 a redirection to a `JSON response`', async ({ - page, -}) => { - await page.goto('/?type="return-redirect"'); + await page.goto('/?type=returnRedirect'); const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); From 2fae5d2de6f5b29e71e066cda8785cf3fc77583c Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 18:55:16 +0100 Subject: [PATCH 6/7] Update test description. --- packages/remix/test/integration/test/client/root-loader.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0a339018eab8..774cbaa3e4c0 100644 --- a/packages/remix/test/integration/test/client/root-loader.test.ts +++ b/packages/remix/test/integration/test/client/root-loader.test.ts @@ -20,7 +20,7 @@ async function extractTraceAndBaggageFromMeta( return { sentryTrace: sentryTraceContent, sentryBaggage: sentryBaggageContent }; } -test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => { +test('should inject `sentry-trace` and `baggage` into root loader returning an empty object.', async ({ page }) => { await page.goto('/?type=empty'); const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page); From 57fb6563903b478c1257700da3677402264e5ad8 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 9 Aug 2022 19:22:29 +0100 Subject: [PATCH 7/7] Skip injection when the response is primitive. --- packages/remix/src/utils/instrumentServer.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index a82b5551f9f5..2a498ec9e6d2 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -280,6 +280,9 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction { if (typeof data === 'object') { return { ...data, ...traceAndBaggage }; + } else { + __DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response body is not an object'); + return data; } }