makeWrappedRootLoader does not handle Response cases

[augustuswm]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/remix

SDK Version

7.9.0

Framework Version

React 17.0.2

Link to Sentry event

No response

Steps to Reproduce

1. Create a Remix project with a root loader that returns a response object (or uses the Remix provided json function).

export const loader: LoaderFunction = async () => {
  return json(
    { data_one: [], data_two: "a string" },
    { headers: { 'Cache-Control': 'max-age=300' } }
  )
}

2. Configure Sentry via Sentry.init(...) in entry.server.tsx
3. Load the root page in a browser
4. Inspect __remixContext.routeData.root in dev tools and it will have the shape { sentryBaggage: string, sentryTrace: string, size: number } instead of containing the loader data.

This looks to be caused by the makeWrappedRootLoader function which handles the result of the origLoader as an Promise<AppData> | AppData instead of Promise<Response> | Response | Promise<AppData> | AppData:

function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
  return async function (this: unknown, args: DataFunctionArgs): Promise<Response | AppData> {
    const res = await origLoader.call(this, args);

    return { ...res, ...getTraceAndBaggage() };
  };
}

Expected Result

Data and ResponseInit settings are propagated when a Promise<Response> or Response is returned from the root loader.

Actual Result

Data and headers (or ResponseInit) data from the root loader are omitted from data on the client and the response respectively.

[onurtemizkan]

Thanks a lot for reporting this @augustuswm! Opened a PR to fix this 👍

[AbhiPrasad]

@augustuswm @massimopalmieri Thank you for the bug reports! The fix for this will be included in the next release (next 1-2 days)!

[augustuswm]

Thanks for getting a fix for this. One note, looking at the PR, is that Remix allows for user defined headers to be included in the Response that is returned from a loader. Currently by transforming the Response into a plain object these settings are lost. The current fix makes sense to unblock anyone that is immediately stuck, but a more complete fix probably looks something like:

function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
  return async function (this: unknown, args: DataFunctionArgs): Promise<Response | AppData> {
    const res = await origLoader.call(this, args);
    const traceAndBaggage = getTraceAndBaggage();

    // 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') {

        // A new response needs to be constructed as the original body can not be replaced
        return new Response(
          JSON.stringify({ ...data, ...traceAndBaggage }),
          { status: res.status, statusText: res.statusText, headers: res.headers }
        );
      } else {
        __DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response body is not an object');
        // Return the response as-is without any changes
        return res;
      }
    }

    return { ...res, ...traceAndBaggage };
  };
}

[AbhiPrasad]

Hey @augustuswm, great suggestion - will use that!

[AbhiPrasad]

Hey folks, fix should be released with https://github.com/getsentry/sentry-javascript/releases/tag/7.10.0! Thanks for your patience.

[Moorst]

Hi, I'm finding this is still occurring for me on Remix when I return a redirect in the root loader. For example:

export const loader = async ({ request }: LoaderArgs) => {
  const user = await getUser(request, userSession)
  const url = new URL(request.url)

  if (!user && url.pathname !== '/sign-in') {
      return redirect('/sign-in')
  }

  return json({user})
}

Here the redirect fails to execute and I get an error stating that user is undefined in the root App component. This is because the loader data is in the shape that the original poster described: { sentryBaggage: string, sentryTrace: string, size: number }.

Interestingly, other redirects I have in route loaders seem to work fine.

This is on version 7.44.2, implemented as suggested in the documentation.