Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(remix): Resolve Remix Request API compatibility issues. #6215

Merged
merged 1 commit into from Nov 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/remix/src/performance/client.tsx
@@ -1,4 +1,4 @@
import { ErrorBoundaryProps, WINDOW , withErrorBoundary } from '@sentry/react';
import { ErrorBoundaryProps, WINDOW, withErrorBoundary } from '@sentry/react';
import { Transaction, TransactionContext } from '@sentry/types';
import { logger } from '@sentry/utils';
import * as React from 'react';
Expand Down
16 changes: 13 additions & 3 deletions packages/remix/src/utils/instrumentServer.ts
Expand Up @@ -21,12 +21,14 @@ import {
DataFunctionArgs,
HandleDocumentRequestFunction,
ReactRouterDomPkg,
RemixRequest,
RequestHandler,
RouteMatch,
ServerBuild,
ServerRoute,
ServerRouteManifest,
} from './types';
import { normalizeRemixRequest } from './web-fetch';

// Flag to track if the core request handler is instrumented.
export let isRequestHandlerWrapped = false;
Expand Down Expand Up @@ -91,8 +93,10 @@ async function captureRemixServerException(err: Error, name: string, request: Re
return;
}

const normalizedRequest = normalizeRemixRequest(request as unknown as any);

captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
scope.setSDKProcessingMetadata({ request });
scope.setSDKProcessingMetadata({ request: normalizedRequest });
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
type: 'instrument',
Expand Down Expand Up @@ -252,6 +256,7 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
// 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 json(
{ ...data, ...traceAndBaggage },
Expand Down Expand Up @@ -363,15 +368,20 @@ export function getTransactionName(
function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
const routes = createRoutes(build.routes);
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {

return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise<Response> {
const local = domain.create();
return local.bind(async () => {
const hub = getCurrentHub();
const options = hub.getClient()?.getOptions();
const scope = hub.getScope();

const normalizedRequest = normalizeRemixRequest(request);

if (scope) {
scope.setSDKProcessingMetadata({ request });
scope.setSDKProcessingMetadata({
request: normalizedRequest,
});
}

if (!options || !hasTracingEnabled(options)) {
Expand Down
21 changes: 18 additions & 3 deletions packages/remix/src/utils/types.ts
Expand Up @@ -3,11 +3,26 @@
// Types vendored from @remix-run/server-runtime@1.6.0:
// https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts
import type * as Express from 'express';
import { Agent } from 'https';
import type { ComponentType } from 'react';

export type RemixRequestState = {
method: string;
redirect: RequestRedirect;
headers: Headers;
parsedURL: URL;
signal: AbortSignal | null;
size: number | null;
};

export type RemixRequest = Request &
Record<symbol | string, RemixRequestState> & {
agent: Agent | ((parsedURL: URL) => Agent) | undefined;
};

export type AppLoadContext = any;
export type AppData = any;
export type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise<Response>;
export type RequestHandler = (request: RemixRequest, loadContext?: AppLoadContext) => Promise<Response>;
export type CreateRequestHandlerFunction = (this: unknown, build: ServerBuild, ...args: any[]) => RequestHandler;
export type ServerRouteManifest = RouteManifest<Omit<ServerRoute, 'children'>>;
export type Params<Key extends string = string> = {
Expand Down Expand Up @@ -104,7 +119,7 @@ export interface ServerBuild {
}

export interface HandleDocumentRequestFunction {
(request: Request, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
(request: RemixRequest, responseStatusCode: number, responseHeaders: Headers, context: EntryContext):
| Promise<Response>
| Response;
}
Expand All @@ -119,7 +134,7 @@ interface ServerEntryModule {
}

export interface DataFunctionArgs {
request: Request;
request: RemixRequest;
context: AppLoadContext;
params: Params;
}
Expand Down
122 changes: 122 additions & 0 deletions packages/remix/src/utils/web-fetch.ts
@@ -0,0 +1,122 @@
// Based on Remix's implementation of Fetch API
// https://github.com/remix-run/web-std-io/tree/main/packages/fetch

import { RemixRequest } from './types';

/*
* Symbol extractor utility to be able to access internal fields of Remix requests.
*/
const getInternalSymbols = (
request: Record<string, unknown>,
): {
bodyInternalsSymbol: string;
requestInternalsSymbol: string;
} => {
const symbols = Object.getOwnPropertySymbols(request);
return {
bodyInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Body internals')) as any,
requestInternalsSymbol: symbols.find(symbol => symbol.toString().includes('Request internals')) as any,
};
};

/**
* Vendored from:
* https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/utils/get-search.js#L5
*/
export const getSearch = (parsedURL: URL): string => {
if (parsedURL.search) {
return parsedURL.search;
}

const lastOffset = parsedURL.href.length - 1;
const hash = parsedURL.hash || (parsedURL.href[lastOffset] === '#' ? '#' : '');
return parsedURL.href[lastOffset - hash.length] === '?' ? '?' : '';
};

/**
* Convert a Request to Node.js http request options.
* The options object to be passed to http.request
* Vendored / modified from:
* https://github.com/remix-run/web-std-io/blob/f715b354c8c5b8edc550c5442dec5712705e25e7/packages/fetch/src/request.js#L259
*/
export const normalizeRemixRequest = (request: RemixRequest): Record<string, any> => {
const { requestInternalsSymbol, bodyInternalsSymbol } = getInternalSymbols(request);

if (!requestInternalsSymbol) {
throw new Error('Could not find request internals symbol');
}

const { parsedURL } = request[requestInternalsSymbol];
const headers = new Headers(request[requestInternalsSymbol].headers);

// Fetch step 1.3
if (!headers.has('Accept')) {
headers.set('Accept', '*/*');
}

// HTTP-network-or-cache fetch steps 2.4-2.7
let contentLengthValue = null;
if (request.body === null && /^(post|put)$/i.test(request.method)) {
contentLengthValue = '0';
}

if (request.body !== null) {
const totalBytes = request[bodyInternalsSymbol].size;
// Set Content-Length if totalBytes is a number (that is not NaN)
if (typeof totalBytes === 'number' && !Number.isNaN(totalBytes)) {
contentLengthValue = String(totalBytes);
}
}

if (contentLengthValue) {
headers.set('Content-Length', contentLengthValue);
}

// HTTP-network-or-cache fetch step 2.11
if (!headers.has('User-Agent')) {
headers.set('User-Agent', 'node-fetch');
}

// HTTP-network-or-cache fetch step 2.15
if (request.compress && !headers.has('Accept-Encoding')) {
headers.set('Accept-Encoding', 'gzip,deflate,br');
}

let { agent } = request;

if (typeof agent === 'function') {
agent = agent(parsedURL);
}

if (!headers.has('Connection') && !agent) {
headers.set('Connection', 'close');
}

// HTTP-network fetch step 4.2
// chunked encoding is handled by Node.js
const search = getSearch(parsedURL);

// Manually spread the URL object instead of spread syntax
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this your choice or just copied from their implementation? (Just curious why the spread operator is being avoided.)

Same question with the @ts-ignores below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of the above is a blocker (just my own curiosity), so I'm going to go ahead and merge this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are copied from the original implementation. Not sure what's the reason for spread operator there, but it seems they have kept the approach of node-fetch.

const requestOptions = {
path: parsedURL.pathname + search,
pathname: parsedURL.pathname,
hostname: parsedURL.hostname,
protocol: parsedURL.protocol,
port: parsedURL.port,
hash: parsedURL.hash,
search: parsedURL.search,
// @ts-ignore - it does not has a query
query: parsedURL.query,
href: parsedURL.href,
method: request.method,
// @ts-ignore - not sure what this supposed to do
headers: headers[Symbol.for('nodejs.util.inspect.custom')](),
insecureHTTPParser: request.insecureHTTPParser,
agent,

// [SENTRY] For compatibility with Sentry SDK RequestData parser, adding `originalUrl` property.
originalUrl: parsedURL.href,
};

return requestOptions;
};
15 changes: 15 additions & 0 deletions packages/remix/test/integration/test/server/action.test.ts
Expand Up @@ -33,6 +33,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});
});
Expand Down Expand Up @@ -101,6 +106,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});

Expand All @@ -116,6 +126,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
request: {
method: 'POST',
url,
cookies: expect.any(Object),
headers: {
'user-agent': expect.any(String),
host: 'localhost:8000',
},
},
});
});
Expand Down