Skip to content

fix(nextjs): Don't rexport default in route handlers #8924

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 5, 2023
Merged

fix(nextjs): Don't rexport default in route handlers #8924

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0bd69f3
fix(nextjs): Don't rexport default in route handlers
Aug 31, 2023
d479611
.
Aug 31, 2023
4c5c563
This is straight up cursed man.
Aug 31, 2023
c8417dc
Merge branch 'develop' into lforst-fix-nextjs-default-export
Sep 1, 2023
37339ac
Update packages/nextjs/src/config/loaders/wrappingLoader.ts
Sep 5, 2023
5913e4c
rm comment
Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 93 additions & 76 deletions packages/nextjs/src/config/loaders/wrappingLoader.ts
View file
Open in desktop
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 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
},
});

// 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
18 changes: 10 additions & 8 deletions packages/nextjs/src/config/templates/routeHandlerWrapperTemplate.ts
View file
Open in desktop
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,20 @@ function wrapHandler<T>(handler: T, method: 'GET' | 'POST' | 'PUT' | 'PATCH' | '
});
}

// 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.
Copy link

Choose a reason for hiding this comment

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

Just a bystander reading this but maybe you can remove the " above" bit at the end since the comment has moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yea. Thanks <3

// @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
Open in desktop
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__';