Skip to content

Commit

Permalink
fix(nextjs): Don't rexport default in route handlers (#8924)
Browse files Browse the repository at this point in the history 
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
  • Loading branch information
@lforst @Lms24
lforst and Lms24 authored Sep 5, 2023
1 parent 29ef20e commit b57e139
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 92 deletions.
169 changes: 93 additions & 76 deletions packages/nextjs/src/config/loaders/wrappingLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { stringMatchesSomePattern } from '@sentry/utils';
import * as chalk from 'chalk';
import * as fs from 'fs';
import * as path from 'path';
import type { RollupBuild, RollupError } from 'rollup';
import { rollup } from 'rollup';

import type { VercelCronsConfig } from '../../common/types';
Expand Down Expand Up @@ -277,85 +278,101 @@ async function wrapUserCode(
userModuleCode: string,
userModuleSourceMap: any,
): Promise<{ code: string; map?: any }> {
const rollupBuild = await rollup({
input: SENTRY_WRAPPER_MODULE_NAME,

plugins: [
// We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to
// mess around with file paths and so that we can pass the original user module source map to rollup so that
// rollup gives us a bundle with correct source mapping to the original file
{
name: 'virtualize-sentry-wrapper-modules',
resolveId: id => {
if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) {
return id;
} else {
return null;
}
},
load(id) {
if (id === SENTRY_WRAPPER_MODULE_NAME) {
return wrapperCode;
} else if (id === WRAPPING_TARGET_MODULE_NAME) {
return {
code: userModuleCode,
map: userModuleSourceMap, // give rollup acces to original user module source map
};
} else {
return null;
}
const wrap = (withDefaultExport: boolean): Promise<RollupBuild> =>
rollup({
input: SENTRY_WRAPPER_MODULE_NAME,

plugins: [
// We're using a simple custom plugin that virtualizes our wrapper module and the user module, so we don't have to
// mess around with file paths and so that we can pass the original user module source map to rollup so that
// rollup gives us a bundle with correct source mapping to the original file
{
name: 'virtualize-sentry-wrapper-modules',
resolveId: id => {
if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) {
return id;
} else {
return null;
}
},
load(id) {
if (id === SENTRY_WRAPPER_MODULE_NAME) {
return withDefaultExport ? wrapperCode : wrapperCode.replace('export { default } from', 'export {} from');
} else if (id === WRAPPING_TARGET_MODULE_NAME) {
return {
code: userModuleCode,
map: userModuleSourceMap, // give rollup acces to original user module source map
};
} else {
return null;
}
},
},

// People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to
// handle that correctly so we let a plugin to take care of bundling cjs exports for us.
commonjs({
sourceMap: true,
strictRequires: true, // Don't hoist require statements that users may define
ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context`
ignore() {
// We basically only want to use this plugin for handling the case where users export their handlers with module.exports.
// This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin.
// (Also, modifying require may break user code)
return true;
},
}),
],

// We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external.
external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME,

// Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the
// user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and
// https://stackoverflow.com/a/60347490.)
context: 'this',

// Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root
// level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what
// seems to happen is this:
//
// - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'`
// - Rollup converts '../../utils/helper' into an absolute path
// - We mark the helper module as external
// - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is
// the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the
// bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped
// live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the
// root. Unclear why it's not.)
// - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'`
// rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in
// nextjs..
//
// Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of
// externals entirely, with the result that their paths remain untouched (which is what we want).
makeAbsoluteExternalsRelative: false,
onwarn: (_warning, _warn) => {
// Suppress all warnings - we don't want to bother people with this output
// Might be stuff like "you have unused imports"
// _warn(_warning); // uncomment to debug
},
});

// People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to
// handle that correctly so we let a plugin to take care of bundling cjs exports for us.
commonjs({
sourceMap: true,
strictRequires: true, // Don't hoist require statements that users may define
ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context`
ignore() {
// We want basically only want to use this plugin for handling the case where users export their handlers with module.exports.
// This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin.
// (Also, modifying require may break user code)
return true;
},
}),
],

// We only want to bundle our wrapper module and the wrappee module into one, so we mark everything else as external.
external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME,

// Prevent rollup from stressing out about TS's use of global `this` when polyfilling await. (TS will polyfill if the
// user's tsconfig `target` is set to anything before `es2017`. See https://stackoverflow.com/a/72822340 and
// https://stackoverflow.com/a/60347490.)
context: 'this',

// Rollup's path-resolution logic when handling re-exports can go wrong when wrapping pages which aren't at the root
// level of the `pages` directory. This may be a bug, as it doesn't match the behavior described in the docs, but what
// seems to happen is this:
//
// - We try to wrap `pages/xyz/userPage.js`, which contains `export { helperFunc } from '../../utils/helper'`
// - Rollup converts '../../utils/helper' into an absolute path
// - We mark the helper module as external
// - Rollup then converts it back to a relative path, but relative to `pages/` rather than `pages/xyz/`. (This is
// the part which doesn't match the docs. They say that Rollup will use the common ancestor of all modules in the
// bundle as the basis for the relative path calculation, but both our temporary file and the page being wrapped
// live in `pages/xyz/`, and they're the only two files in the bundle, so `pages/xyz/`` should be used as the
// root. Unclear why it's not.)
// - As a result of the miscalculation, our proxy module will include `export { helperFunc } from '../utils/helper'`
// rather than the expected `export { helperFunc } from '../../utils/helper'`, thereby causing a build error in
// nextjs..
//
// Setting `makeAbsoluteExternalsRelative` to `false` prevents all of the above by causing Rollup to ignore imports of
// externals entirely, with the result that their paths remain untouched (which is what we want).
makeAbsoluteExternalsRelative: false,

onwarn: (_warning, _warn) => {
// Suppress all warnings - we don't want to bother people with this output
// Might be stuff like "you have unused imports"
// _warn(_warning); // uncomment to debug
},
});
// Next.js sometimes complains if you define a default export (e.g. in route handlers in dev mode).
// This is why we want to avoid unnecessarily creating default exports, even if they're just `undefined`.
// For this reason we try to bundle/wrap the user code once including a re-export of `default`.
// If the user code didn't have a default export, rollup will throw.
// We then try bundling/wrapping agian, but without including a re-export of `default`.
let rollupBuild;
try {
rollupBuild = await wrap(true);
} catch (e) {
if ((e as RollupError)?.code === 'MISSING_EXPORT') {
rollupBuild = await wrap(false);
} else {
throw e;
}
}

const finalBundle = await rollupBuild.generate({
format: 'esm',
Expand Down
16 changes: 8 additions & 8 deletions packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import type { RequestAsyncStorage } from './requestAsyncStorageShim';
declare const requestAsyncStorage: RequestAsyncStorage;

declare const routeModule: {
default: unknown;
GET?: (...args: unknown[]) => unknown;
POST?: (...args: unknown[]) => unknown;
PUT?: (...args: unknown[]) => unknown;
Expand Down Expand Up @@ -59,17 +58,18 @@ function wrapHandler<T>(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | '
});
}

// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
export * from '__SENTRY_WRAPPING_TARGET_FILE__';

// @ts-ignore This is the file we're wrapping
// eslint-disable-next-line import/no-unresolved
export { default } from '__SENTRY_WRAPPING_TARGET_FILE__';

export const GET = wrapHandler(routeModule.GET, 'GET');
export const POST = wrapHandler(routeModule.POST, 'POST');
export const PUT = wrapHandler(routeModule.PUT, 'PUT');
export const PATCH = wrapHandler(routeModule.PATCH, 'PATCH');
export const DELETE = wrapHandler(routeModule.DELETE, 'DELETE');
export const HEAD = wrapHandler(routeModule.HEAD, 'HEAD');
export const OPTIONS = wrapHandler(routeModule.OPTIONS, 'OPTIONS');

// Re-export anything exported by the page module we're wrapping. When processing this code, Rollup is smart enough to
// not include anything whose name matchs something we've explicitly exported above.
// @ts-ignore See above
// eslint-disable-next-line import/no-unresolved
export * from '__SENTRY_WRAPPING_TARGET_FILE__';
export default routeModule.default;
10 changes: 2 additions & 8 deletions packages/nextjs/src/config/templates/sentryInitWrapperTemplate.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,8 @@ import '__SENTRY_CONFIG_IMPORT_PATH__';

// @ts-ignore This is the file we're wrapping
// eslint-disable-next-line import/no-unresolved
import * as wrappee from '__SENTRY_WRAPPING_TARGET_FILE__';

// @ts-ignore default either exists, or it doesn't - we don't care
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const defaultExport = wrappee.default;
export * from '__SENTRY_WRAPPING_TARGET_FILE__';

// @ts-ignore This is the file we're wrapping
// eslint-disable-next-line import/no-unresolved
export * from '__SENTRY_WRAPPING_TARGET_FILE__';

export default defaultExport;
export { default } from '__SENTRY_WRAPPING_TARGET_FILE__';

0 comments on commit b57e139

Please sign in to comment.