From ef89101a5d2e966dacd725ef252c23d42c24841f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 9 Aug 2022 11:03:16 +0000 Subject: [PATCH 01/25] feat(nextjs): Wrap server-side getInitialProps --- packages/nextjs/src/config/loaders/ast.ts | 163 +++++++++++++++++- .../src/config/loaders/dataFetchersLoader.ts | 104 +++++++---- .../nextjs/src/config/loaders/prefixLoader.ts | 2 +- packages/nextjs/src/config/loaders/types.ts | 20 ++- .../templates/dataFetchersLoaderTemplate.ts | 8 +- packages/nextjs/src/config/webpack.ts | 4 +- packages/nextjs/src/config/wrappers/index.ts | 7 +- packages/nextjs/src/config/wrappers/types.ts | 11 +- .../src/config/wrappers/withSentryGSPaths.ts | 16 -- .../src/config/wrappers/withSentryGSProps.ts | 16 -- .../src/config/wrappers/withSentryGSSP.ts | 16 -- .../wrappers/withSentryGetInitialProps.ts | 18 ++ .../wrappers/withSentryGetServerSideProps.ts | 14 ++ .../wrappers/withSentryGetStaticPaths.ts | 14 ++ .../wrappers/withSentryGetStaticProps.ts | 14 ++ packages/nextjs/src/index.client.ts | 2 + packages/nextjs/src/index.server.ts | 7 +- packages/nextjs/test/config/ast.test.ts | 100 +++++++++++ 18 files changed, 436 insertions(+), 100 deletions(-) delete mode 100644 packages/nextjs/src/config/wrappers/withSentryGSPaths.ts delete mode 100644 packages/nextjs/src/config/wrappers/withSentryGSProps.ts delete mode 100644 packages/nextjs/src/config/wrappers/withSentryGSSP.ts create mode 100644 packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts create mode 100644 packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts create mode 100644 packages/nextjs/src/config/wrappers/withSentryGetStaticPaths.ts create mode 100644 packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts create mode 100644 packages/nextjs/test/config/ast.test.ts diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 18f7380553b3..422aa4994384 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -33,8 +33,12 @@ const { ObjectExpression, ObjectPattern, Property, - VariableDeclaration, VariableDeclarator, + VariableDeclaration, + ExportNamedDeclaration, + ExportAllDeclaration, + ExportDefaultDeclaration, + ExportDefaultSpecifier, } = jscs; type ASTNode = jscsTypes.ASTNode; @@ -320,3 +324,160 @@ export function removeComments(ast: AST): void { const nodesWithComments = ast.find(Node).filter(nodePath => !!nodePath.node.comments); nodesWithComments.forEach(nodePath => (nodePath.node.comments = null)); } + +/** + * TODO + */ +export function hasDefaultExport(ast: AST): boolean { + const hasDefaultDeclaration = ast.find(ExportDefaultDeclaration).size() > 0; + if (hasDefaultDeclaration) { + return true; + } + + const hasDefaultSpecifier = ast.find(ExportDefaultSpecifier).size() > 0; + if (hasDefaultSpecifier) { + return true; + } + + const hasNamedDefaultExport = ast + .find(ExportSpecifier) + .nodes() + .some(specifier => specifier.exported.name === 'default'); + if (hasNamedDefaultExport) { + return true; + } + + return false; +} + +/** + * TODO + */ +function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { + const identifiers: string[] = []; + + if (restElement.argument.type === 'Identifier') { + identifiers.push(restElement.argument.name); + } else if (restElement.argument.type === 'ArrayPattern') { + identifiers.push(...getExportIdentifiersFromArrayPattern(restElement.argument)); + } else if (restElement.argument.type === 'ObjectPattern') { + identifiers.push(...getExportIdentifiersFromObjectPattern(restElement.argument)); + } else if (restElement.argument.type === 'RestElement') { + identifiers.push(...getExportIdentifiersFromRestElement(restElement.argument)); + } + + return identifiers; +} + +/** + * TODO + */ +function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPattern): string[] { + const identifiers: string[] = []; + + arrayPattern.elements.forEach(element => { + if (element?.type === 'Identifier') { + identifiers.push(element.name); + } else if (element?.type === 'ObjectPattern') { + identifiers.push(...getExportIdentifiersFromObjectPattern(element)); + } else if (element?.type === 'ArrayPattern') { + identifiers.push(...getExportIdentifiersFromArrayPattern(element)); + } else if (element?.type === 'RestElement') { + identifiers.push(...getExportIdentifiersFromRestElement(element)); + } + }); + + return identifiers; +} + +/** + * TODO + */ +function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.ObjectPattern): string[] { + const identifiers: string[] = []; + + objectPatternNode.properties.forEach(property => { + if (property.type === 'Property') { + if (property.value.type === 'Identifier') { + identifiers.push(property.value.name); + } else if (property.value.type === 'ObjectPattern') { + identifiers.push(...getExportIdentifiersFromObjectPattern(property.value)); + } else if (property.value.type === 'ArrayPattern') { + identifiers.push(...getExportIdentifiersFromArrayPattern(property.value)); + } else if (property.value.type === 'RestElement') { + identifiers.push(...getExportIdentifiersFromRestElement(property.value)); + } + // @ts-ignore seems to be a bug in the jscs typing + } else if (property.type === 'RestElement') { + // @ts-ignore seems to be a bug in the jscs typing + identifiers.push(...getExportIdentifiersFromRestElement(property)); + } + }); + + return identifiers; +} + +/** + * TODO + */ +export function getExportIdentifiers(ast: AST): string[] { + const identifiers: string[] = []; + + const namedExportDeclarationNodes = ast + .find(ExportNamedDeclaration) + .nodes() + .map(namedExportDeclarationNode => namedExportDeclarationNode.declaration); + + namedExportDeclarationNodes + .filter( + (declarationNode): declarationNode is jscsTypes.VariableDeclaration => + declarationNode !== null && declarationNode.type === 'VariableDeclaration', + ) + .map(variableDeclarationNode => variableDeclarationNode.declarations) + .reduce((prev, curr) => [...prev, ...curr], []) // flatten + .forEach(declarationNode => { + if (declarationNode.type === 'Identifier' || declarationNode.type === 'JSXIdentifier') { + identifiers.push(declarationNode.name); + } else if (declarationNode.type === 'TSTypeParameter') { + // noop + } else if (declarationNode.id.type === 'Identifier') { + identifiers.push(declarationNode.id.name); + } else if (declarationNode.id.type === 'ObjectPattern') { + identifiers.push(...getExportIdentifiersFromObjectPattern(declarationNode.id)); + } else if (declarationNode.id.type === 'ArrayPattern') { + identifiers.push(...getExportIdentifiersFromArrayPattern(declarationNode.id)); + } else if (declarationNode.id.type === 'RestElement') { + identifiers.push(...getExportIdentifiersFromRestElement(declarationNode.id)); + } + }); + + namedExportDeclarationNodes + .filter( + (declarationNode): declarationNode is jscsTypes.ClassDeclaration | jscsTypes.FunctionDeclaration => + declarationNode !== null && + (declarationNode.type === 'ClassDeclaration' || declarationNode.type === 'FunctionDeclaration'), + ) + .map(node => node.id) + .filter((id): id is jscsTypes.Identifier => id !== null && id.type === 'Identifier') + .forEach(id => identifiers.push(id.name)); + + ast + .find(ExportSpecifier) + .nodes() + .forEach(specifier => { + if (specifier.exported.name !== 'default') { + identifiers.push(specifier.exported.name); + } + }); + + ast + .find(ExportAllDeclaration) + .nodes() + .forEach(declaration => { + if (declaration.exported) { + identifiers.push(declaration.exported.name); + } + }); + + return [...new Set(identifiers)]; +} diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 2758a0f49807..aef354dd4600 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -6,15 +6,26 @@ * manipulating them, and then turning them back into strings and appending our template code to the user's (modified) * page code. Greater detail and explanations can be found in situ in the functions below and in the helper functions in * `ast.ts`. + * + * For `getInitialProps` we create a virtual proxy-module that re-exports all the exports and default exports of the + * original file and wraps `getInitialProps`. We do this since it allows us to very generically wrap `getInitialProps` + * for all kinds ways users might define default exports (which are a lot of ways). */ - import { logger } from '@sentry/utils'; import * as fs from 'fs'; import * as path from 'path'; import { isESM } from '../../utils/isESM'; import type { AST } from './ast'; -import { findDeclarations, findExports, makeAST, removeComments, renameIdentifiers } from './ast'; +import { + findDeclarations, + findExports, + getExportIdentifiers, + hasDefaultExport, + makeAST, + removeComments, + renameIdentifiers, +} from './ast'; import type { LoaderThis } from './types'; // Map to keep track of each function's placeholder in the template and what it should be replaced with. (The latter @@ -94,44 +105,73 @@ function wrapFunctions(userCode: string, templateCode: string, filepath: string) * Wrap `getStaticPaths`, `getStaticProps`, and `getServerSideProps` (if they exist) in the given page code */ export default function wrapDataFetchersLoader(this: LoaderThis, userCode: string): string { - // We know one or the other will be defined, depending on the version of webpack being used - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const { projectDir } = this.getOptions ? this.getOptions() : this.query!; - // For now this loader only works for ESM code if (!isESM(userCode)) { return userCode; } - // If none of the functions we want to wrap appears in the page's code, there's nothing to do. (Note: We do this as a - // simple substring match (rather than waiting until we've parsed the code) because it's meant to be an - // as-fast-as-possible fail-fast. It's possible for user code to pass this check, even if it contains none of the - // functions in question, just by virtue of the correct string having been found, be it in a comment, as part of a - // longer variable name, etc. That said, when we actually do the code manipulation we'll be working on the code's AST, - // meaning we'll be able to differentiate between code we actually want to change and any false positives which might - // come up here.) - if (Object.keys(DATA_FETCHING_FUNCTIONS).every(functionName => !userCode.includes(functionName))) { - return userCode; - } + // We know one or the other will be defined, depending on the version of webpack being used + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const { projectDir } = 'getOptions' in this ? this.getOptions() : this.query; - const templatePath = path.resolve(__dirname, '../templates/dataFetchersLoaderTemplate.js'); - // make sure the template is included when runing `webpack watch` - this.addDependency(templatePath); + // Proxy the processed file + if (!this.resourceQuery.includes('sentry-proxy-loader')) { + const ast = makeAST(userCode, true); // is there a reason to ever parse without typescript? - const templateCode = fs.readFileSync(templatePath).toString(); + const exportedIdentifiers = getExportIdentifiers(ast); - const [modifiedUserCode, modifiedTemplateCode] = wrapFunctions( - userCode, - templateCode, - // Relative path to the page we're currently processing, for use in error messages - path.relative(projectDir, this.resourcePath), - ); + let outputFileContent = ''; - // Fill in template placeholders - let injectedCode = modifiedTemplateCode; - for (const { placeholder, alias } of Object.values(DATA_FETCHING_FUNCTIONS)) { - injectedCode = injectedCode.replace(placeholder, alias); - } + if (exportedIdentifiers.length > 0) { + outputFileContent += `export { ${exportedIdentifiers.join(', ')} } from "${ + this.resourcePath + }?sentry-proxy-loader";`; + } - return `${modifiedUserCode}\n${injectedCode}`; + if (hasDefaultExport(ast)) { + outputFileContent += ` + import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; + import { withSentryGetInitialProps } from "@sentry/nextjs/build/esm/config/wrappers"; + Object.defineProperty( + _sentry_default, + 'getInitialProps', + { value: withSentryGetInitialProps(_sentry_default.getInitialProps) } + ); + export default _sentry_default;`; + } + + return outputFileContent; + } else { + // If none of the functions we want to wrap appears in the page's code, there's nothing to do. (Note: We do this as a + // simple substring match (rather than waiting until we've parsed the code) because it's meant to be an + // as-fast-as-possible fail-fast. It's possible for user code to pass this check, even if it contains none of the + // functions in question, just by virtue of the correct string having been found, be it in a comment, as part of a + // longer variable name, etc. That said, when we actually do the code manipulation we'll be working on the code's AST, + // meaning we'll be able to differentiate between code we actually want to change and any false positives which might + // come up here.) + if (Object.keys(DATA_FETCHING_FUNCTIONS).every(functionName => !userCode.includes(functionName))) { + return userCode; + } + + const templatePath = path.resolve(__dirname, '../templates/dataFetchersLoaderTemplate.js'); + // make sure the template is included when runing `webpack watch` + this.addDependency(templatePath); + + const templateCode = fs.readFileSync(templatePath).toString(); + + const [modifiedUserCode, modifiedTemplateCode] = wrapFunctions( + userCode, + templateCode, + // Relative path to the page we're currently processing, for use in error messages + path.relative(projectDir, this.resourcePath), + ); + + // Fill in template placeholders + let injectedCode = modifiedTemplateCode; + for (const { placeholder, alias } of Object.values(DATA_FETCHING_FUNCTIONS)) { + injectedCode = injectedCode.replace(new RegExp(placeholder, 'g'), alias); + } + + return `${modifiedUserCode}\n${injectedCode}`; + } } diff --git a/packages/nextjs/src/config/loaders/prefixLoader.ts b/packages/nextjs/src/config/loaders/prefixLoader.ts index 668ca194dfbb..def107840511 100644 --- a/packages/nextjs/src/config/loaders/prefixLoader.ts +++ b/packages/nextjs/src/config/loaders/prefixLoader.ts @@ -13,7 +13,7 @@ type LoaderOptions = { export default function prefixLoader(this: LoaderThis, userCode: string): string { // We know one or the other will be defined, depending on the version of webpack being used // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const { distDir } = this.getOptions ? this.getOptions() : this.query!; + const { distDir } = 'getOptions' in this ? this.getOptions() : this.query; const templatePath = path.resolve(__dirname, '../templates/prefixLoaderTemplate.js'); // make sure the template is included when runing `webpack watch` diff --git a/packages/nextjs/src/config/loaders/types.ts b/packages/nextjs/src/config/loaders/types.ts index 17f7a737c72a..5db902abeced 100644 --- a/packages/nextjs/src/config/loaders/types.ts +++ b/packages/nextjs/src/config/loaders/types.ts @@ -1,13 +1,19 @@ -// TODO Use real webpack types export type LoaderThis = { - // Path to the file being loaded + /** Path to the file being loaded */ resourcePath: string; - // Loader options in Webpack 4 - query?: Options; - // Loader options in Webpack 5 - getOptions?: () => Options; + /** Query at the end of resolved file name ("../some-folder/some-module?foobar" -> resourceQuery: "?foobar") */ + resourceQuery: string; // Function to add outside file used by loader to `watch` process addDependency: (filepath: string) => void; -}; +} & ( + | { + // Loader options in Webpack 4 + query: Options; + } + | { + // Loader options in Webpack 5 + getOptions: () => Options; + } +); diff --git a/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts b/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts index b3fc03c45091..3d3d26994307 100644 --- a/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts +++ b/packages/nextjs/src/config/templates/dataFetchersLoaderTemplate.ts @@ -23,12 +23,14 @@ declare const __ORIG_GSPATHS__: GetStaticPathsFunction; import * as ServerSideSentryNextjsSDK from '@sentry/nextjs'; export const getServerSideProps = - typeof __ORIG_GSSP__ === 'function' ? ServerSideSentryNextjsSDK.withSentryGSSP(__ORIG_GSSP__) : __ORIG_GSSP__; + typeof __ORIG_GSSP__ === 'function' + ? ServerSideSentryNextjsSDK.withSentryGetServerSideProps(__ORIG_GSSP__) + : __ORIG_GSSP__; export const getStaticProps = typeof __ORIG_GSPROPS__ === 'function' - ? ServerSideSentryNextjsSDK.withSentryGSProps(__ORIG_GSPROPS__) + ? ServerSideSentryNextjsSDK.withSentryGetStaticProps(__ORIG_GSPROPS__) : __ORIG_GSPROPS__; export const getStaticPaths = typeof __ORIG_GSPATHS__ === 'function' - ? ServerSideSentryNextjsSDK.withSentryGSPaths(__ORIG_GSPATHS__) + ? ServerSideSentryNextjsSDK.withSentryGetStaticPaths(__ORIG_GSPATHS__) : __ORIG_GSPATHS__; diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index 4b750fbe1c6e..ea120b64d76a 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -43,7 +43,7 @@ export function constructWebpackConfigFunction( // Will be called by nextjs and passed its default webpack configuration and context data about the build (whether // we're building server or client, whether we're in dev, what version of webpack we're using, etc). Note that // `incomingConfig` and `buildContext` are referred to as `config` and `options` in the nextjs docs. - const newWebpackFunction = (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => { + return (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => { const { isServer, dev: isDev, dir: projectDir } = buildContext; let newConfig = { ...incomingConfig }; @@ -165,8 +165,6 @@ export function constructWebpackConfigFunction( return newConfig; }; - - return newWebpackFunction; } /** diff --git a/packages/nextjs/src/config/wrappers/index.ts b/packages/nextjs/src/config/wrappers/index.ts index ee3fdcdd8f2f..6ef54a3001a3 100644 --- a/packages/nextjs/src/config/wrappers/index.ts +++ b/packages/nextjs/src/config/wrappers/index.ts @@ -1,3 +1,4 @@ -export { withSentryGSPaths } from './withSentryGSPaths'; -export { withSentryGSProps } from './withSentryGSProps'; -export { withSentryGSSP } from './withSentryGSSP'; +export { withSentryGetStaticPaths } from './withSentryGetStaticPaths'; +export { withSentryGetStaticProps } from './withSentryGetStaticProps'; +export { withSentryGetInitialProps } from './withSentryGetInitialProps'; +export { withSentryGetServerSideProps } from './withSentryGetServerSideProps'; diff --git a/packages/nextjs/src/config/wrappers/types.ts b/packages/nextjs/src/config/wrappers/types.ts index 5587aa1a71f7..f407ae72c53d 100644 --- a/packages/nextjs/src/config/wrappers/types.ts +++ b/packages/nextjs/src/config/wrappers/types.ts @@ -8,6 +8,8 @@ import type { GetStaticProps, GetStaticPropsContext, GetStaticPropsResult, + NextPage, + NextPageContext, } from 'next'; type Paths = { [key: string]: string | string[] }; @@ -34,4 +36,11 @@ export type GSSP = { result: GetServerSidePropsResult; }; -export type DataFetchingFunction = GSPaths | GSProps | GSSP; +export type GIProps = { + fn: Required['getInitialProps']; + wrappedFn: NextPage['getInitialProps']; + context: NextPageContext; + result: unknown; +}; + +export type DataFetchingFunction = GSPaths | GSProps | GSSP | GIProps; diff --git a/packages/nextjs/src/config/wrappers/withSentryGSPaths.ts b/packages/nextjs/src/config/wrappers/withSentryGSPaths.ts deleted file mode 100644 index 7161f81f9780..000000000000 --- a/packages/nextjs/src/config/wrappers/withSentryGSPaths.ts +++ /dev/null @@ -1,16 +0,0 @@ -import type { GSPaths } from './types'; -import { callOriginal } from './wrapperUtils'; - -/** - * Create a wrapped version of the user's exported `getStaticPaths` function - * - * @param origGSPaths: The user's `getStaticPaths` function - * @returns A wrapped version of the function - */ -export function withSentryGSPaths(origGSPaths: GSPaths['fn']): GSPaths['wrappedFn'] { - const wrappedGSPaths = async function (context: GSPaths['context']): Promise { - return callOriginal(origGSPaths, context); - }; - - return wrappedGSPaths; -} diff --git a/packages/nextjs/src/config/wrappers/withSentryGSProps.ts b/packages/nextjs/src/config/wrappers/withSentryGSProps.ts deleted file mode 100644 index 7121de35f28b..000000000000 --- a/packages/nextjs/src/config/wrappers/withSentryGSProps.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { GSProps } from './types'; -import { callOriginal } from './wrapperUtils'; - -/** - * Create a wrapped version of the user's exported `getStaticProps` function - * - * @param origGSProps: The user's `getStaticProps` function - * @returns A wrapped version of the function - */ -export function withSentryGSProps(origGSProps: GSProps['fn']): GSProps['wrappedFn'] { - const wrappedGSProps = async function (context: GSProps['context']): Promise { - return callOriginal(origGSProps, context); - }; - - return wrappedGSProps; -} diff --git a/packages/nextjs/src/config/wrappers/withSentryGSSP.ts b/packages/nextjs/src/config/wrappers/withSentryGSSP.ts deleted file mode 100644 index ddcc8f8035ca..000000000000 --- a/packages/nextjs/src/config/wrappers/withSentryGSSP.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { GSSP } from './types'; -import { callOriginal } from './wrapperUtils'; - -/** - * Create a wrapped version of the user's exported `getServerSideProps` function - * - * @param origGSSP: The user's `getServerSideProps` function - * @returns A wrapped version of the function - */ -export function withSentryGSSP(origGSSP: GSSP['fn']): GSSP['wrappedFn'] { - const wrappedGSSP = async function (context: GSSP['context']): Promise { - return callOriginal(origGSSP, context); - }; - - return wrappedGSSP; -} diff --git a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts new file mode 100644 index 000000000000..954aa6506002 --- /dev/null +++ b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts @@ -0,0 +1,18 @@ +import { GIProps } from './types'; + +/** + * Create a wrapped version of the user's exported `getInitialProps` function + * + * @param origGIProps: The user's `getInitialProps` function + * @param origGIPropsHost: The user's object on which `getInitialProps` lives (used for `this`) + * @returns A wrapped version of the function + */ +export function withSentryGetInitialProps(origGIProps: GIProps['fn'] | undefined): GIProps['wrappedFn'] { + if (typeof origGIProps === 'function') { + return async function (this: unknown, ...args: Parameters) { + return await origGIProps.call(this, ...args); + }; + } else { + return origGIProps; + } +} diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts new file mode 100644 index 000000000000..0293ad999c45 --- /dev/null +++ b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts @@ -0,0 +1,14 @@ +import { GSSP } from './types'; +import { callOriginal } from './wrapperUtils'; + +/** + * Create a wrapped version of the user's exported `getServerSideProps` function + * + * @param origGetServerSideProps: The user's `getServerSideProps` function + * @returns A wrapped version of the function + */ +export function withSentryGetServerSideProps(origGetServerSideProps: GSSP['fn']): GSSP['wrappedFn'] { + return async function (context: GSSP['context']): Promise { + return callOriginal(origGetServerSideProps, context); + }; +} diff --git a/packages/nextjs/src/config/wrappers/withSentryGetStaticPaths.ts b/packages/nextjs/src/config/wrappers/withSentryGetStaticPaths.ts new file mode 100644 index 000000000000..0c821c2940f2 --- /dev/null +++ b/packages/nextjs/src/config/wrappers/withSentryGetStaticPaths.ts @@ -0,0 +1,14 @@ +import type { GSPaths } from './types'; +import { callOriginal } from './wrapperUtils'; + +/** + * Create a wrapped version of the user's exported `getStaticPaths` function + * + * @param origGetStaticPaths: The user's `getStaticPaths` function + * @returns A wrapped version of the function + */ +export function withSentryGetStaticPaths(origGetStaticPaths: GSPaths['fn']): GSPaths['wrappedFn'] { + return async function (context: GSPaths['context']): Promise { + return callOriginal(origGetStaticPaths, context); + }; +} diff --git a/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts new file mode 100644 index 000000000000..23d6023a25bf --- /dev/null +++ b/packages/nextjs/src/config/wrappers/withSentryGetStaticProps.ts @@ -0,0 +1,14 @@ +import { GSProps } from './types'; +import { callOriginal } from './wrapperUtils'; + +/** + * Create a wrapped version of the user's exported `getStaticProps` function + * + * @param origGetStaticProps: The user's `getStaticProps` function + * @returns A wrapped version of the function + */ +export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn']): GSProps['wrappedFn'] { + return async function (context: GSProps['context']): Promise { + return callOriginal(origGetStaticProps, context); + }; +} diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index a7bfca1f2660..1ff546c5248d 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -11,6 +11,8 @@ export * from '@sentry/react'; export { nextRouterInstrumentation } from './performance/client'; export { captureUnderscoreErrorException } from './utils/_error'; +export { withSentryGetInitialProps } from './config/wrappers'; + export { Integrations }; // Previously we expected users to import `BrowserTracing` like this: diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index ef398eb49d8f..914fe26168ce 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -125,7 +125,12 @@ function addServerIntegrations(options: NextjsOptions): void { export type { SentryWebpackPluginOptions } from './config/types'; export { withSentryConfig } from './config'; export { isBuild } from './utils/isBuild'; -export { withSentryGSProps, withSentryGSSP, withSentryGSPaths } from './config/wrappers'; +export { + withSentryGetServerSideProps, + withSentryGetStaticProps, + withSentryGetStaticPaths, + withSentryGetInitialProps, +} from './config/wrappers'; export { withSentry } from './utils/withSentry'; // Wrap various server methods to enable error monitoring and tracing. (Note: This only happens for non-Vercel diff --git a/packages/nextjs/test/config/ast.test.ts b/packages/nextjs/test/config/ast.test.ts new file mode 100644 index 000000000000..c80076ef48af --- /dev/null +++ b/packages/nextjs/test/config/ast.test.ts @@ -0,0 +1,100 @@ +import { getExportIdentifiers, hasDefaultExport, makeAST } from '../../src/config/loaders/ast'; + +test.each([ + // examples taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export + // Exporting declarations + ['export let name1, name2; export var name3, name4;', false], + ['export const name1 = 1, name2 = 2;', false], + ['export var name1 = 1, name2 = 2;', false], + ['export let name1 = 1, name2 = 2;', false], + ['export function functionName() {}', false], + ['export class ClassName {}', false], + ['export function* generatorFunctionName() {}', false], + ['export const { name1, bar: name2, someValue: { someNestedValue: name3 }, ...name4 } = {};', false], + ['export const [ name1, name2, ...name3 ] = [1, 2, 3, 4];', false], + ['export const { foo: { bar: [{ baz: [name1, ...name2], ...name3 }, name4, name5]} } = {};', false], + ['export const [{ a: { ...name1 }, b: [,name2] }, name3] = [];', false], + // Export list + ['var name1, name2, name3; export { name1, name2, name3 };', false], + ['var variable1, variable2, name3; export { variable1 as name1, variable2 as name2, name3 };', false], + ['var name1, name2, name3; export { name1 as default, name1, name2 };', true], + // Default exports + ['export default 1;', true], + ['export default function functionName() {}', true], + ['export default class ClassName {}', true], + ['export default function* generatorFunctionName() {}', true], + ['export default function () {}', true], + ['export default class {}', true], + ['export default function* () {}', true], + ['const someObj = { a: { b: 1 }}; export default a.b', true], + // Aggregating modules + ['export * from "module-name";', false], + ['export * as name1 from "module-name";', false], + ['export { name1, name2 } from "module-name";', false], + ['export { import1 as name1, import2 as name2, name3 } from "module-name";', false], + ['export { default } from "module-name";', true], + ['export { default, name1 } from "module-name";', true], +])('hasDefaultExport(%s) should return %p', (program, expectedResult) => { + const ast = makeAST(program, true); + expect(hasDefaultExport(ast)).toStrictEqual(expectedResult); +}); + +test.each([ + // examples taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export + // Exporting declarations + ['export let name1, name2; export var name3, name4;', ['name1', 'name2', 'name3', 'name4']], + ['export const name1 = 1, name2 = 2;', ['name1', 'name2']], + ['export var name1 = 1, name2 = 2;', ['name1', 'name2']], + ['export let name1 = 1, name2 = 2;', ['name1', 'name2']], + ['export function functionName() {}', ['functionName']], + ['export class ClassName {}', ['ClassName']], + ['export function* generatorFunctionName() {}', ['generatorFunctionName']], + [ + 'export const { name1, bar: name2, someValue: { someNestedValue: name3 }, ...name4 } = {};', + ['name1', 'name2', 'name3', 'name4'], + ], + ['export const [ name1, name2, ...name3 ] = [1, 2, 3, 4];', ['name1', 'name2', 'name3']], + [ + 'export const { foo: { bar: [{ baz: [name1, ...name2], ...name3 }, name4, name5]} } = {};', + ['name1', 'name2', 'name3', 'name4', 'name5'], + ], + ['export const [{ a: { ...name1 }, b: [,name2] }, name3] = [];', ['name1', 'name2', 'name3']], + // Export list + [ + ` + var name1, name2, name3; + export { name1, name2, name3 };`, + ['name1', 'name2', 'name3'], + ], + [ + ` + var variable1, variable2, name3; + export { variable1 as name1, variable2 as name2, name3 };`, + ['name1', 'name2', 'name3'], + ], + [ + ` + var name1, name2, name3; + export { name1 as default, name1, name2 };`, + ['name1', 'name2'], + ], + // Default exports + ['export default 1;', []], + ['export default function functionName() {}', []], + ['export default class ClassName {}', []], + ['export default function* generatorFunctionName() {}', []], + ['export default function () {}', []], + ['export default class {}', []], + ['export default function* () {}', []], + ['const someObj = { a: { b: 1 }}; export default a.b', []], + // Aggregating modules + ['export * from "module-name";', []], + ['export * as name1 from "module-name";', ['name1']], + ['export { name1, name2 } from "module-name";', ['name1', 'name2']], + ['export { import1 as name1, import2 as name2, name3 } from "module-name";', ['name1', 'name2', 'name3']], + ['export { default } from "module-name";', []], + ['export { default, name1 } from "module-name";', ['name1']], +])('getExportIdentifiers(%s) should return %p', (program, expectedIdentifiers) => { + const ast = makeAST(program, true); + expect(getExportIdentifiers(ast)).toStrictEqual(expectedIdentifiers); +}); From 56aa62e340d4fd499a52fdd96736b2d0441c005e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 9 Aug 2022 12:50:40 +0000 Subject: [PATCH 02/25] Add type guards --- packages/nextjs/src/config/loaders/ast.ts | 71 +++++++++++------------ 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 422aa4994384..90bd3b1b2b0b 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -25,20 +25,26 @@ const jscs = jscodeshiftDefault || jscodeshiftNamespace; // These are types not in the TS sense, but in the instance-of-a-Type-class sense const { - ExportSpecifier, + Node, Identifier, - ImportSpecifier, + JSXIdentifier, MemberExpression, - Node, + ClassDeclaration, + FunctionDeclaration, ObjectExpression, ObjectPattern, + ArrayPattern, + RestElement, Property, VariableDeclarator, VariableDeclaration, + ImportSpecifier, + ExportSpecifier, ExportNamedDeclaration, ExportAllDeclaration, ExportDefaultDeclaration, ExportDefaultSpecifier, + TSTypeParameter, } = jscs; type ASTNode = jscsTypes.ASTNode; @@ -295,8 +301,6 @@ function maybeRenameNode(ast: AST, identifierPath: ASTPath, alia // it means we potentially need to rename something *not* already named `getServerSideProps`, `getStaticProps`, or // `getStaticPaths`, meaning we need to rename nodes outside of the collection upon which we're currently acting. if (ExportSpecifier.check(parent)) { - // console.log(node); - // debugger; if (parent.exported.name !== parent.local?.name && node === parent.exported) { const currentLocalName = parent.local?.name || ''; renameIdentifiers(ast, currentLocalName, alias); @@ -339,10 +343,7 @@ export function hasDefaultExport(ast: AST): boolean { return true; } - const hasNamedDefaultExport = ast - .find(ExportSpecifier) - .nodes() - .some(specifier => specifier.exported.name === 'default'); + const hasNamedDefaultExport = ast.find(ExportSpecifier).some(specifier => specifier.node.exported.name === 'default'); if (hasNamedDefaultExport) { return true; } @@ -356,13 +357,13 @@ export function hasDefaultExport(ast: AST): boolean { function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { const identifiers: string[] = []; - if (restElement.argument.type === 'Identifier') { + if (Identifier.check(restElement.argument)) { identifiers.push(restElement.argument.name); - } else if (restElement.argument.type === 'ArrayPattern') { + } else if (ArrayPattern.check(restElement.argument)) { identifiers.push(...getExportIdentifiersFromArrayPattern(restElement.argument)); - } else if (restElement.argument.type === 'ObjectPattern') { + } else if (ObjectPattern.check(restElement.argument)) { identifiers.push(...getExportIdentifiersFromObjectPattern(restElement.argument)); - } else if (restElement.argument.type === 'RestElement') { + } else if (RestElement.check(restElement.argument)) { identifiers.push(...getExportIdentifiersFromRestElement(restElement.argument)); } @@ -376,13 +377,13 @@ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPatte const identifiers: string[] = []; arrayPattern.elements.forEach(element => { - if (element?.type === 'Identifier') { + if (Identifier.check(element)) { identifiers.push(element.name); - } else if (element?.type === 'ObjectPattern') { + } else if (ObjectPattern.check(element)) { identifiers.push(...getExportIdentifiersFromObjectPattern(element)); - } else if (element?.type === 'ArrayPattern') { + } else if (ArrayPattern.check(element)) { identifiers.push(...getExportIdentifiersFromArrayPattern(element)); - } else if (element?.type === 'RestElement') { + } else if (RestElement.check(element)) { identifiers.push(...getExportIdentifiersFromRestElement(element)); } }); @@ -397,19 +398,17 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje const identifiers: string[] = []; objectPatternNode.properties.forEach(property => { - if (property.type === 'Property') { - if (property.value.type === 'Identifier') { + if (Property.check(property)) { + if (Identifier.check(property.value)) { identifiers.push(property.value.name); - } else if (property.value.type === 'ObjectPattern') { + } else if (ObjectPattern.check(property.value)) { identifiers.push(...getExportIdentifiersFromObjectPattern(property.value)); - } else if (property.value.type === 'ArrayPattern') { + } else if (ArrayPattern.check(property.value)) { identifiers.push(...getExportIdentifiersFromArrayPattern(property.value)); - } else if (property.value.type === 'RestElement') { + } else if (RestElement.check(property.value)) { identifiers.push(...getExportIdentifiersFromRestElement(property.value)); } - // @ts-ignore seems to be a bug in the jscs typing - } else if (property.type === 'RestElement') { - // @ts-ignore seems to be a bug in the jscs typing + } else if (RestElement.check(property)) { identifiers.push(...getExportIdentifiersFromRestElement(property)); } }); @@ -429,24 +428,23 @@ export function getExportIdentifiers(ast: AST): string[] { .map(namedExportDeclarationNode => namedExportDeclarationNode.declaration); namedExportDeclarationNodes - .filter( - (declarationNode): declarationNode is jscsTypes.VariableDeclaration => - declarationNode !== null && declarationNode.type === 'VariableDeclaration', + .filter((declarationNode): declarationNode is jscsTypes.VariableDeclaration => + VariableDeclaration.check(declarationNode), ) .map(variableDeclarationNode => variableDeclarationNode.declarations) .reduce((prev, curr) => [...prev, ...curr], []) // flatten .forEach(declarationNode => { - if (declarationNode.type === 'Identifier' || declarationNode.type === 'JSXIdentifier') { + if (Identifier.check(declarationNode) || JSXIdentifier.check(declarationNode)) { identifiers.push(declarationNode.name); - } else if (declarationNode.type === 'TSTypeParameter') { + } else if (TSTypeParameter.check(declarationNode)) { // noop - } else if (declarationNode.id.type === 'Identifier') { + } else if (Identifier.check(declarationNode.id)) { identifiers.push(declarationNode.id.name); - } else if (declarationNode.id.type === 'ObjectPattern') { + } else if (ObjectPattern.check(declarationNode.id)) { identifiers.push(...getExportIdentifiersFromObjectPattern(declarationNode.id)); - } else if (declarationNode.id.type === 'ArrayPattern') { + } else if (ArrayPattern.check(declarationNode.id)) { identifiers.push(...getExportIdentifiersFromArrayPattern(declarationNode.id)); - } else if (declarationNode.id.type === 'RestElement') { + } else if (RestElement.check(declarationNode.id)) { identifiers.push(...getExportIdentifiersFromRestElement(declarationNode.id)); } }); @@ -454,11 +452,10 @@ export function getExportIdentifiers(ast: AST): string[] { namedExportDeclarationNodes .filter( (declarationNode): declarationNode is jscsTypes.ClassDeclaration | jscsTypes.FunctionDeclaration => - declarationNode !== null && - (declarationNode.type === 'ClassDeclaration' || declarationNode.type === 'FunctionDeclaration'), + ClassDeclaration.check(declarationNode) || FunctionDeclaration.check(declarationNode), ) .map(node => node.id) - .filter((id): id is jscsTypes.Identifier => id !== null && id.type === 'Identifier') + .filter((id): id is jscsTypes.Identifier => Identifier.check(id)) .forEach(id => identifiers.push(id.name)); ast From a08158224eefbcf89db8acb25aa8ea23a8177a4d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 9 Aug 2022 13:56:19 +0000 Subject: [PATCH 03/25] Add jsdoc --- packages/nextjs/src/config/loaders/ast.ts | 40 ++++++++++++++++------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 90bd3b1b2b0b..cf879ac896d6 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -330,7 +330,7 @@ export function removeComments(ast: AST): void { } /** - * TODO + * Determines from a given AST of a file, whether the file has a default export or not. */ export function hasDefaultExport(ast: AST): boolean { const hasDefaultDeclaration = ast.find(ExportDefaultDeclaration).size() > 0; @@ -352,26 +352,32 @@ export function hasDefaultExport(ast: AST): boolean { } /** - * TODO + * Extracts all identifiers from a "RestElement" within a named export declaration statement ("export const val = ..."). + * "RestElements" are things like "...value" inside a destructuring assignment. + * + * Example: + * "export const { ...restElement1, bar: { ...restElement2 } } = { foo: 1, bar: { baz: 2 } }" --> ["restElement1", "restElement2"] */ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { const identifiers: string[] = []; if (Identifier.check(restElement.argument)) { identifiers.push(restElement.argument.name); - } else if (ArrayPattern.check(restElement.argument)) { - identifiers.push(...getExportIdentifiersFromArrayPattern(restElement.argument)); - } else if (ObjectPattern.check(restElement.argument)) { - identifiers.push(...getExportIdentifiersFromObjectPattern(restElement.argument)); - } else if (RestElement.check(restElement.argument)) { - identifiers.push(...getExportIdentifiersFromRestElement(restElement.argument)); } return identifiers; } /** - * TODO + * Extracts all identifiers from an ArrayPattern within a destructured named export declaration + * statement ("export const [val] = [1]"). + * + * This function recursively calls itself and `getExportIdentifiersFromObjectPattern` since destructuring assignments + * can be deeply nested with objects and arrays. + * + * Example: + * export const [{ foo: name1 }, [{ bar: [name2]}, name3]] = [{ foo: 1 }, [{ bar: [2] }, 3]]; + * --> ["name1", "name2", "name3"] */ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPattern): string[] { const identifiers: string[] = []; @@ -392,7 +398,15 @@ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPatte } /** - * TODO + * Grabs all identifiers from an ObjectPattern within a destructured named export declaration + * statement ("export const { val: name } = { val: 1 }"). + * + * This function recursively calls itself and `getExportIdentifiersFromArrayPattern` since destructuring assignments + * can be deeply nested with objects and arrays. + * + * Example: + * export const { foo: [name1], bar: { baz: [{ quux: name2 }], ...name3 }} = { foo: [1], bar: { baz: [{ quux: 3 }]} }; + * --> ["name1", "name2", "name3"] */ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.ObjectPattern): string[] { const identifiers: string[] = []; @@ -417,7 +431,9 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje } /** - * TODO + * Given the AST of a file, this function extracts all named exports from the file. + * + * @returns a list of deduplicated identifiers. */ export function getExportIdentifiers(ast: AST): string[] { const identifiers: string[] = []; @@ -476,5 +492,5 @@ export function getExportIdentifiers(ast: AST): string[] { } }); - return [...new Set(identifiers)]; + return [...new Set(identifiers)]; // dedupe } From 916fdb0246d44bb53f52c3bb631166724ba7c143 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 10:20:26 +0200 Subject: [PATCH 04/25] Apply comment wording suggestions Co-authored-by: Katie Byers --- packages/nextjs/src/config/loaders/ast.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index cf879ac896d6..8a466af6a4f1 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -330,7 +330,7 @@ export function removeComments(ast: AST): void { } /** - * Determines from a given AST of a file, whether the file has a default export or not. + * Determines from a given AST of a file whether the file has a default export or not. */ export function hasDefaultExport(ast: AST): boolean { const hasDefaultDeclaration = ast.find(ExportDefaultDeclaration).size() > 0; @@ -352,8 +352,8 @@ export function hasDefaultExport(ast: AST): boolean { } /** - * Extracts all identifiers from a "RestElement" within a named export declaration statement ("export const val = ..."). - * "RestElements" are things like "...value" inside a destructuring assignment. + * Extracts all identifiers from a `RestElement` within a named export declaration statement (`export const constName = constValue`). + * `RestElements` are things like `...others` inside a destructuring assignment. * * Example: * "export const { ...restElement1, bar: { ...restElement2 } } = { foo: 1, bar: { baz: 2 } }" --> ["restElement1", "restElement2"] @@ -412,6 +412,7 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje const identifiers: string[] = []; objectPatternNode.properties.forEach(property => { + // An `ObjectPattern`'s properties can be either `Property`s or `RestElement`s. if (Property.check(property)) { if (Identifier.check(property.value)) { identifiers.push(property.value.name); @@ -436,6 +437,7 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje * @returns a list of deduplicated identifiers. */ export function getExportIdentifiers(ast: AST): string[] { + // We'll use a set to dedupe at the end, but for now we use an array as our accumulator because you can add multiple elements to it at once. const identifiers: string[] = []; const namedExportDeclarationNodes = ast From baa0a5ad6f6ace8f6deceefae5857a3096536af8 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 09:05:27 +0000 Subject: [PATCH 05/25] Simplify and speed up hasDefaultExport --- packages/nextjs/src/config/loaders/ast.ts | 24 +++++++++-------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 8a466af6a4f1..236281fe0d33 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -333,22 +333,16 @@ export function removeComments(ast: AST): void { * Determines from a given AST of a file whether the file has a default export or not. */ export function hasDefaultExport(ast: AST): boolean { - const hasDefaultDeclaration = ast.find(ExportDefaultDeclaration).size() > 0; - if (hasDefaultDeclaration) { - return true; - } - - const hasDefaultSpecifier = ast.find(ExportDefaultSpecifier).size() > 0; - if (hasDefaultSpecifier) { - return true; - } - - const hasNamedDefaultExport = ast.find(ExportSpecifier).some(specifier => specifier.node.exported.name === 'default'); - if (hasNamedDefaultExport) { - return true; - } + const defaultExports = ast.find(Node, value => { + return ( + ExportDefaultDeclaration.check(value) || + ExportDefaultSpecifier.check(value) || + (ExportSpecifier.check(value) && value.exported.name === 'default') + ); + }); - return false; + // In theory there should only ever be 0 or 1, but who knows what people do + return defaultExports.length > 0; } /** From 145ed92912d472c33bcf3ec7df8aa502327c41c3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 09:20:02 +0000 Subject: [PATCH 06/25] Clean up `getExportIdentifiersFromRestElement` description --- packages/nextjs/src/config/loaders/ast.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 236281fe0d33..da867fa637dc 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -346,11 +346,19 @@ export function hasDefaultExport(ast: AST): boolean { } /** - * Extracts all identifiers from a `RestElement` within a named export declaration statement (`export const constName = constValue`). + * Extracts all identifiers from a `RestElement` within a named export declaration statement (`export const constName = constValue;`). * `RestElements` are things like `...others` inside a destructuring assignment. * - * Example: - * "export const { ...restElement1, bar: { ...restElement2 } } = { foo: 1, bar: { baz: 2 } }" --> ["restElement1", "restElement2"] + * Example - take the following program: + * + * ```js + * export const { bar: { ...restElement1 }, ...restElement2 } = { foo: { baz: 1 }, bar: 2 }; + * ``` + * + * The `RestElement` nodes in this program are "...restElement1" and "...restElement2". This function takes such nodes + * and gets their identifier names. + * - `"...restElemet1"` --> `"restElement1"` + * - `"...restElemet2"` --> `"restElement2"` */ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { const identifiers: string[] = []; From ad46cdb1739e69da2fe96761cc5d2ea2d47d08ac Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 09:27:13 +0000 Subject: [PATCH 07/25] Sorted imports --- packages/nextjs/src/config/loaders/ast.ts | 26 +++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index da867fa637dc..6a913ba20d91 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -25,26 +25,26 @@ const jscs = jscodeshiftDefault || jscodeshiftNamespace; // These are types not in the TS sense, but in the instance-of-a-Type-class sense const { - Node, + ArrayPattern, + ClassDeclaration, + ExportAllDeclaration, + ExportDefaultDeclaration, + ExportDefaultSpecifier, + ExportNamedDeclaration, + ExportSpecifier, + FunctionDeclaration, Identifier, + ImportSpecifier, JSXIdentifier, MemberExpression, - ClassDeclaration, - FunctionDeclaration, + Node, ObjectExpression, ObjectPattern, - ArrayPattern, - RestElement, Property, - VariableDeclarator, - VariableDeclaration, - ImportSpecifier, - ExportSpecifier, - ExportNamedDeclaration, - ExportAllDeclaration, - ExportDefaultDeclaration, - ExportDefaultSpecifier, + RestElement, TSTypeParameter, + VariableDeclaration, + VariableDeclarator, } = jscs; type ASTNode = jscsTypes.ASTNode; From 460f11362fc5c8bd42bf3c2478c31bfa5eafd0f3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 09:51:17 +0000 Subject: [PATCH 08/25] Simplify `getExportIdentifiersFromRestElement` --- packages/nextjs/src/config/loaders/ast.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 6a913ba20d91..b555c6b8489f 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -352,22 +352,19 @@ export function hasDefaultExport(ast: AST): boolean { * Example - take the following program: * * ```js - * export const { bar: { ...restElement1 }, ...restElement2 } = { foo: { baz: 1 }, bar: 2 }; + * export const { bar: { ...restElement1 }, ...restElement2 } = { foo: { baz: 1 }, bar: 2 }; * ``` * * The `RestElement` nodes in this program are "...restElement1" and "...restElement2". This function takes such nodes * and gets their identifier names. + * * - `"...restElemet1"` --> `"restElement1"` * - `"...restElemet2"` --> `"restElement2"` */ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { - const identifiers: string[] = []; - - if (Identifier.check(restElement.argument)) { - identifiers.push(restElement.argument.name); - } - - return identifiers; + // This function returns an array instead of `string | undefined` for convenience since we exclusively use the return + // value with `array.push()` in other functions in this file. + return Identifier.check(restElement.argument) ? [restElement.argument.name] : []; } /** From 0002873d52758bb79d0c5f9d4b5c8c55f63541ce Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 09:58:46 +0000 Subject: [PATCH 09/25] Clean up `getExportIdentifiersFromArrayPattern` description --- packages/nextjs/src/config/loaders/ast.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index b555c6b8489f..82f9aceba643 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -368,15 +368,21 @@ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement) } /** - * Extracts all identifiers from an ArrayPattern within a destructured named export declaration - * statement ("export const [val] = [1]"). + * Extracts all identifier names (`'constName'`) from an destructuringassignment'sArrayPattern (the `[constName]` in`const [constName] = [1]`). * * This function recursively calls itself and `getExportIdentifiersFromObjectPattern` since destructuring assignments * can be deeply nested with objects and arrays. * - * Example: + * Example - take the following program: + * + * ```js * export const [{ foo: name1 }, [{ bar: [name2]}, name3]] = [{ foo: 1 }, [{ bar: [2] }, 3]]; - * --> ["name1", "name2", "name3"] + * ``` + * + * The `ArrayPattern` node in question for this program is the left hand side of the assignment: + * `[{ foo: name1 }, [{ bar: [name2]}, name3]]` + * + * Applying this function to this `ArrayPattern` will return the following: `["name1", "name2", "name3"]` */ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPattern): string[] { const identifiers: string[] = []; From aef96355a90c0112e9daddb0b73345bb7ba655b6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 10:15:07 +0000 Subject: [PATCH 10/25] Rename `getExportIdentifiers` to `getExportIdentifierNames` --- packages/nextjs/src/config/loaders/ast.ts | 2 +- packages/nextjs/src/config/loaders/dataFetchersLoader.ts | 4 ++-- packages/nextjs/test/config/ast.test.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 82f9aceba643..a63bcd24b769 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -441,7 +441,7 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje * * @returns a list of deduplicated identifiers. */ -export function getExportIdentifiers(ast: AST): string[] { +export function getExportIdentifierNames(ast: AST): string[] { // We'll use a set to dedupe at the end, but for now we use an array as our accumulator because you can add multiple elements to it at once. const identifiers: string[] = []; diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index aef354dd4600..59776d3be4c3 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -20,7 +20,7 @@ import type { AST } from './ast'; import { findDeclarations, findExports, - getExportIdentifiers, + getExportIdentifierNames, hasDefaultExport, makeAST, removeComments, @@ -118,7 +118,7 @@ export default function wrapDataFetchersLoader(this: LoaderThis, if (!this.resourceQuery.includes('sentry-proxy-loader')) { const ast = makeAST(userCode, true); // is there a reason to ever parse without typescript? - const exportedIdentifiers = getExportIdentifiers(ast); + const exportedIdentifiers = getExportIdentifierNames(ast); let outputFileContent = ''; diff --git a/packages/nextjs/test/config/ast.test.ts b/packages/nextjs/test/config/ast.test.ts index c80076ef48af..95dae93eb619 100644 --- a/packages/nextjs/test/config/ast.test.ts +++ b/packages/nextjs/test/config/ast.test.ts @@ -1,4 +1,4 @@ -import { getExportIdentifiers, hasDefaultExport, makeAST } from '../../src/config/loaders/ast'; +import { getExportIdentifierNames, hasDefaultExport, makeAST } from '../../src/config/loaders/ast'; test.each([ // examples taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export @@ -96,5 +96,5 @@ test.each([ ['export { default, name1 } from "module-name";', ['name1']], ])('getExportIdentifiers(%s) should return %p', (program, expectedIdentifiers) => { const ast = makeAST(program, true); - expect(getExportIdentifiers(ast)).toStrictEqual(expectedIdentifiers); + expect(getExportIdentifierNames(ast)).toStrictEqual(expectedIdentifiers); }); From 41ebd28084dc7c79461541d04bc994d0c30fc264 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 11:39:23 +0000 Subject: [PATCH 11/25] Rename `namedExportDeclarationNodes` to `namedExportDeclarationNodeDeclarations` --- packages/nextjs/src/config/loaders/ast.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index a63bcd24b769..48545d1d4de6 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -445,12 +445,12 @@ export function getExportIdentifierNames(ast: AST): string[] { // We'll use a set to dedupe at the end, but for now we use an array as our accumulator because you can add multiple elements to it at once. const identifiers: string[] = []; - const namedExportDeclarationNodes = ast + const namedExportDeclarationNodeDeclarations = ast .find(ExportNamedDeclaration) .nodes() .map(namedExportDeclarationNode => namedExportDeclarationNode.declaration); - namedExportDeclarationNodes + namedExportDeclarationNodeDeclarations .filter((declarationNode): declarationNode is jscsTypes.VariableDeclaration => VariableDeclaration.check(declarationNode), ) @@ -472,7 +472,7 @@ export function getExportIdentifierNames(ast: AST): string[] { } }); - namedExportDeclarationNodes + namedExportDeclarationNodeDeclarations .filter( (declarationNode): declarationNode is jscsTypes.ClassDeclaration | jscsTypes.FunctionDeclaration => ClassDeclaration.check(declarationNode) || FunctionDeclaration.check(declarationNode), From 0f1390b7d2f09cb86b7c701b60d471fc106c5409 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 12:14:56 +0000 Subject: [PATCH 12/25] Add comments and eliminate dead code paths --- packages/nextjs/src/config/loaders/ast.ts | 56 +++++++++++++++++------ 1 file changed, 42 insertions(+), 14 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 48545d1d4de6..12acb6967c25 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -445,6 +445,12 @@ export function getExportIdentifierNames(ast: AST): string[] { // We'll use a set to dedupe at the end, but for now we use an array as our accumulator because you can add multiple elements to it at once. const identifiers: string[] = []; + // The following variable collects all export statements that double as named declaration, e.g.: + // - export function myFunc() {} + // - export var myVar = 1337 + // - export const myConst = 1337 + // - export const { a, ..rest } = { a: 1, b: 2, c: 3 } + // We will narrow those situations down in subsequent code blocks. const namedExportDeclarationNodeDeclarations = ast .find(ExportNamedDeclaration) .nodes() @@ -452,50 +458,72 @@ export function getExportIdentifierNames(ast: AST): string[] { namedExportDeclarationNodeDeclarations .filter((declarationNode): declarationNode is jscsTypes.VariableDeclaration => + // Narrow down to varible declarations, e.g.: + // export const a = ...; + // export var b = ...; + // export let c = ...; + // export let c, d = 1; VariableDeclaration.check(declarationNode), ) - .map(variableDeclarationNode => variableDeclarationNode.declarations) - .reduce((prev, curr) => [...prev, ...curr], []) // flatten + .map( + variableDeclarationNode => + // Grab all declarations in a single export statement. + // There can be multiple in the case of for example in `export let a, b;`. + variableDeclarationNode.declarations, + ) + .reduce((prev, curr) => [...prev, ...curr], []) // flatten - now we have all declaration nodes in one flat array .forEach(declarationNode => { - if (Identifier.check(declarationNode) || JSXIdentifier.check(declarationNode)) { - identifiers.push(declarationNode.name); - } else if (TSTypeParameter.check(declarationNode)) { - // noop + if ( + Identifier.check(declarationNode) || // should never happen + JSXIdentifier.check(declarationNode) || // JSX like `` - we don't care about these + TSTypeParameter.check(declarationNode) // type definitions - we don't care about those + ) { + // We should never have to enter this branch, it is just for type narrowing. } else if (Identifier.check(declarationNode.id)) { + // If it's a simple declaration with an identifier we collect it. (e.g. `const myIdentifier = 1;` -> "myIdentifier") identifiers.push(declarationNode.id.name); } else if (ObjectPattern.check(declarationNode.id)) { + // If we encounter a destructuring export like `export const { foo: name1, bar: name2 } = { foo: 1, bar: 2 };`, + // we try collecting the identifiers from the pattern `{ foo: name1, bar: name2 }`. identifiers.push(...getExportIdentifiersFromObjectPattern(declarationNode.id)); } else if (ArrayPattern.check(declarationNode.id)) { + // If we encounter a destructuring export like `export const [name1, name2] = [1, 2];`, + // we try collecting the identifiers from the pattern `[name1, name2]`. identifiers.push(...getExportIdentifiersFromArrayPattern(declarationNode.id)); - } else if (RestElement.check(declarationNode.id)) { - identifiers.push(...getExportIdentifiersFromRestElement(declarationNode.id)); } }); namedExportDeclarationNodeDeclarations .filter( + // Narrow down to class and function declarations, e.g.: + // export class Foo {}; + // export function bar() {}; (declarationNode): declarationNode is jscsTypes.ClassDeclaration | jscsTypes.FunctionDeclaration => ClassDeclaration.check(declarationNode) || FunctionDeclaration.check(declarationNode), ) - .map(node => node.id) - .filter((id): id is jscsTypes.Identifier => Identifier.check(id)) - .forEach(id => identifiers.push(id.name)); + .map(node => node.id) // Grab the identifier of the function/class - Note: it might be `null` when it's anonymous + .filter((id): id is jscsTypes.Identifier => Identifier.check(id)) // Elaborate way of null-checking + .forEach(id => identifiers.push(id.name)); // Collect the name of the identifier ast - .find(ExportSpecifier) + .find(ExportSpecifier) // Find stuff like `export {} [from ...];` .nodes() .forEach(specifier => { + // Taking the example above `specifier.exported.name` always contains `id` unless `name` is specified, then it's `name`; if (specifier.exported.name !== 'default') { + // You can do default exports "export { something as default };" but we do not want to collect "default" in this + // function since it only wants to collect named exports. identifiers.push(specifier.exported.name); } }); ast - .find(ExportAllDeclaration) + .find(ExportAllDeclaration) // Find stuff like `export * from ..." and "export * as someVariable from ...` .nodes() .forEach(declaration => { + // Narrow it down to only find `export * as someVariable from ...` (emphasis on "as someVariable") if (declaration.exported) { - identifiers.push(declaration.exported.name); + identifiers.push(declaration.exported.name); // `declaration.exported.name` contains "someVariable" } }); From 36f44342c4ca228d12ae9c8bab4096de74acc974 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 12:16:29 +0000 Subject: [PATCH 13/25] s/toStrictEqual/toBe/ in test --- packages/nextjs/test/config/ast.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/test/config/ast.test.ts b/packages/nextjs/test/config/ast.test.ts index 95dae93eb619..c6188f49d2ac 100644 --- a/packages/nextjs/test/config/ast.test.ts +++ b/packages/nextjs/test/config/ast.test.ts @@ -36,7 +36,7 @@ test.each([ ['export { default, name1 } from "module-name";', true], ])('hasDefaultExport(%s) should return %p', (program, expectedResult) => { const ast = makeAST(program, true); - expect(hasDefaultExport(ast)).toStrictEqual(expectedResult); + expect(hasDefaultExport(ast)).toBe(expectedResult); }); test.each([ From cd94b1257ac0998c35a28d06bcbb6a0f26a1c27e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 12:18:23 +0000 Subject: [PATCH 14/25] Add name to anonymous function for better debugging --- packages/nextjs/src/config/webpack.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index ea120b64d76a..a636609a9a7b 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -43,7 +43,10 @@ export function constructWebpackConfigFunction( // Will be called by nextjs and passed its default webpack configuration and context data about the build (whether // we're building server or client, whether we're in dev, what version of webpack we're using, etc). Note that // `incomingConfig` and `buildContext` are referred to as `config` and `options` in the nextjs docs. - return (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => { + return function newWebpackFunction( + incomingConfig: WebpackConfigObject, + buildContext: BuildContext, + ): WebpackConfigObject { const { isServer, dev: isDev, dir: projectDir } = buildContext; let newConfig = { ...incomingConfig }; From 7f0c91b72eeea93591918b44917c2fc37bcc2770 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 12:24:51 +0000 Subject: [PATCH 15/25] Remove unnecessary eslint ignores --- packages/nextjs/src/config/loaders/dataFetchersLoader.ts | 1 - packages/nextjs/src/config/loaders/prefixLoader.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 59776d3be4c3..593645ed8536 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -111,7 +111,6 @@ export default function wrapDataFetchersLoader(this: LoaderThis, } // We know one or the other will be defined, depending on the version of webpack being used - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const { projectDir } = 'getOptions' in this ? this.getOptions() : this.query; // Proxy the processed file diff --git a/packages/nextjs/src/config/loaders/prefixLoader.ts b/packages/nextjs/src/config/loaders/prefixLoader.ts index def107840511..6422941c99e5 100644 --- a/packages/nextjs/src/config/loaders/prefixLoader.ts +++ b/packages/nextjs/src/config/loaders/prefixLoader.ts @@ -12,7 +12,6 @@ type LoaderOptions = { */ export default function prefixLoader(this: LoaderThis, userCode: string): string { // We know one or the other will be defined, depending on the version of webpack being used - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const { distDir } = 'getOptions' in this ? this.getOptions() : this.query; const templatePath = path.resolve(__dirname, '../templates/prefixLoaderTemplate.js'); From 4c97ee9089555051b1500ec8a2928da6889570fd Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 13:21:20 +0000 Subject: [PATCH 16/25] Preserve enumerable property --- packages/nextjs/src/config/loaders/dataFetchersLoader.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 593645ed8536..9ac8bc9a2af6 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -134,7 +134,10 @@ export default function wrapDataFetchersLoader(this: LoaderThis, Object.defineProperty( _sentry_default, 'getInitialProps', - { value: withSentryGetInitialProps(_sentry_default.getInitialProps) } + { + ...Object.getOwnPropertyDescriptor(_sentry_default, 'getInitialProps'), + value: withSentryGetInitialProps(_sentry_default.getInitialProps) + } ); export default _sentry_default;`; } From 3ccbe1093e3d906968cae98ce1d60c2458f81d80 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 13:33:39 +0000 Subject: [PATCH 17/25] Add comments clarifying why getInitialProps wrapper is imported directly --- packages/nextjs/src/config/loaders/dataFetchersLoader.ts | 3 +++ packages/nextjs/src/config/wrappers/index.ts | 4 ++++ .../nextjs/src/config/wrappers/withSentryGetInitialProps.ts | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 9ac8bc9a2af6..b3491d892300 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -128,6 +128,9 @@ export default function wrapDataFetchersLoader(this: LoaderThis, } if (hasDefaultExport(ast)) { + // We don't import the `withSentryGetInitialProps` wrapper from `@sentry/nextjs` since `getInitialProps` can run + // on both the server and the client and we don't want to have side effects from the server SDK on the client and + // vice versa. By importing the loader directly we avoid the side effects. outputFileContent += ` import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; import { withSentryGetInitialProps } from "@sentry/nextjs/build/esm/config/wrappers"; diff --git a/packages/nextjs/src/config/wrappers/index.ts b/packages/nextjs/src/config/wrappers/index.ts index 6ef54a3001a3..bcafc255a016 100644 --- a/packages/nextjs/src/config/wrappers/index.ts +++ b/packages/nextjs/src/config/wrappers/index.ts @@ -2,3 +2,7 @@ export { withSentryGetStaticPaths } from './withSentryGetStaticPaths'; export { withSentryGetStaticProps } from './withSentryGetStaticProps'; export { withSentryGetInitialProps } from './withSentryGetInitialProps'; export { withSentryGetServerSideProps } from './withSentryGetServerSideProps'; + +// Disclaimer: Keep this file side-effect free. If you have to intruduce a side-effect, make sure it can run on the +// browser and on the server. Reason: Our `getInitialProps` wrapper imports this file and `getInitialProps` might run on +// the browser and / or on the server. diff --git a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts index 954aa6506002..55882a2e228c 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts @@ -1,5 +1,9 @@ import { GIProps } from './types'; +// Disclaimer: Keep this file side-effect free. If you have to intruduce a side-effect, make sure it can run on the +// browser and on the server. Reason: This `getInitialProps` wrapper imports this file and `getInitialProps` might run on +// the browser and / or on the server. + /** * Create a wrapped version of the user's exported `getInitialProps` function * From 4c3c3cecd4dd6101a1bfa912180e865ae5e54b4c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:27:17 +0000 Subject: [PATCH 18/25] Add comment explaining wht proxying is --- .../nextjs/src/config/loaders/dataFetchersLoader.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index b3491d892300..6c68b49f5460 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -113,7 +113,16 @@ export default function wrapDataFetchersLoader(this: LoaderThis, // We know one or the other will be defined, depending on the version of webpack being used const { projectDir } = 'getOptions' in this ? this.getOptions() : this.query; - // Proxy the processed file + // In the following branch we will proxy the user's file. This means we return code (basically an entirely new file) + // that re - exports all the user file's originial export, but with a "sentry-proxy-loader" query in the module + // string. + // This looks like the following: `export { a, b, c } from "[imagine userfile path here]?sentry-proxy-loader";` + // Additionally, in this proxy file we import the userfile's default export, wrap `getInitialProps` on that default + // export, and re -export the now modified default export as default. + // Webpack will resolve the module with the "sentry-proxy-loader" query to the original file, but will give us access + // to the query via`this.resourceQuery`. If we see that `this.resourceQuery` includes includes "sentry-proxy-loader" + // we know we're in a proxied file and do not need to proxy again. + if (!this.resourceQuery.includes('sentry-proxy-loader')) { const ast = makeAST(userCode, true); // is there a reason to ever parse without typescript? From c0577da4ad77711f31e822dd5dce8490a2f59850 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:31:45 +0000 Subject: [PATCH 19/25] Remove defineProperty call --- .../nextjs/src/config/loaders/dataFetchersLoader.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 6c68b49f5460..250c1f44a2f6 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -142,15 +142,10 @@ export default function wrapDataFetchersLoader(this: LoaderThis, // vice versa. By importing the loader directly we avoid the side effects. outputFileContent += ` import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; - import { withSentryGetInitialProps } from "@sentry/nextjs/build/esm/config/wrappers"; - Object.defineProperty( - _sentry_default, - 'getInitialProps', - { - ...Object.getOwnPropertyDescriptor(_sentry_default, 'getInitialProps'), - value: withSentryGetInitialProps(_sentry_default.getInitialProps) - } - ); + import { withSentryGetInitialProps } from "@sentry/nextjs"; + + _sentry_default.getInitialProps = withSentryGetInitialProps(_sentry_default.getInitialProps); + export default _sentry_default;`; } From ed15ca360121a0635b186ab6fe2555af3fb9afcb Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:38:11 +0000 Subject: [PATCH 20/25] Pull undefined check out of wrapper --- .../nextjs/src/config/loaders/dataFetchersLoader.ts | 4 +++- .../src/config/wrappers/withSentryGetInitialProps.ts | 12 ++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 250c1f44a2f6..237f95032e7d 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -144,7 +144,9 @@ export default function wrapDataFetchersLoader(this: LoaderThis, import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; import { withSentryGetInitialProps } from "@sentry/nextjs"; - _sentry_default.getInitialProps = withSentryGetInitialProps(_sentry_default.getInitialProps); + if (typeof _sentry_default.getInitialProps === 'function') { + _sentry_default.getInitialProps = withSentryGetInitialProps(_sentry_default.getInitialProps); + } export default _sentry_default;`; } diff --git a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts index 55882a2e228c..e1272743db9f 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts @@ -11,12 +11,8 @@ import { GIProps } from './types'; * @param origGIPropsHost: The user's object on which `getInitialProps` lives (used for `this`) * @returns A wrapped version of the function */ -export function withSentryGetInitialProps(origGIProps: GIProps['fn'] | undefined): GIProps['wrappedFn'] { - if (typeof origGIProps === 'function') { - return async function (this: unknown, ...args: Parameters) { - return await origGIProps.call(this, ...args); - }; - } else { - return origGIProps; - } +export function withSentryGetInitialProps(origGIProps: GIProps['fn']): GIProps['wrappedFn'] { + return async function (this: unknown, ...args: Parameters) { + return await origGIProps.call(this, ...args); + }; } From 2f88743be475afc44c89cac2452fade51b9120a7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:41:00 +0000 Subject: [PATCH 21/25] Remove redundant comments --- packages/nextjs/src/config/wrappers/index.ts | 4 ---- .../nextjs/src/config/wrappers/withSentryGetInitialProps.ts | 4 ---- 2 files changed, 8 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/index.ts b/packages/nextjs/src/config/wrappers/index.ts index bcafc255a016..6ef54a3001a3 100644 --- a/packages/nextjs/src/config/wrappers/index.ts +++ b/packages/nextjs/src/config/wrappers/index.ts @@ -2,7 +2,3 @@ export { withSentryGetStaticPaths } from './withSentryGetStaticPaths'; export { withSentryGetStaticProps } from './withSentryGetStaticProps'; export { withSentryGetInitialProps } from './withSentryGetInitialProps'; export { withSentryGetServerSideProps } from './withSentryGetServerSideProps'; - -// Disclaimer: Keep this file side-effect free. If you have to intruduce a side-effect, make sure it can run on the -// browser and on the server. Reason: Our `getInitialProps` wrapper imports this file and `getInitialProps` might run on -// the browser and / or on the server. diff --git a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts index e1272743db9f..7e5e209ae98a 100644 --- a/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts +++ b/packages/nextjs/src/config/wrappers/withSentryGetInitialProps.ts @@ -1,9 +1,5 @@ import { GIProps } from './types'; -// Disclaimer: Keep this file side-effect free. If you have to intruduce a side-effect, make sure it can run on the -// browser and on the server. Reason: This `getInitialProps` wrapper imports this file and `getInitialProps` might run on -// the browser and / or on the server. - /** * Create a wrapped version of the user's exported `getInitialProps` function * From adad5e308bcdfefef5b65a33815506ba9731427a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:42:02 +0000 Subject: [PATCH 22/25] Remove redundant comments --- packages/nextjs/src/config/loaders/dataFetchersLoader.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts index 237f95032e7d..d94811498e79 100644 --- a/packages/nextjs/src/config/loaders/dataFetchersLoader.ts +++ b/packages/nextjs/src/config/loaders/dataFetchersLoader.ts @@ -137,9 +137,6 @@ export default function wrapDataFetchersLoader(this: LoaderThis, } if (hasDefaultExport(ast)) { - // We don't import the `withSentryGetInitialProps` wrapper from `@sentry/nextjs` since `getInitialProps` can run - // on both the server and the client and we don't want to have side effects from the server SDK on the client and - // vice versa. By importing the loader directly we avoid the side effects. outputFileContent += ` import { default as _sentry_default } from "${this.resourcePath}?sentry-proxy-loader"; import { withSentryGetInitialProps } from "@sentry/nextjs"; From aa5b915872d776268bdc71ad6551afde98725561 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 15:54:56 +0000 Subject: [PATCH 23/25] Add disclaimiers why functions can only be used in an export context --- packages/nextjs/src/config/loaders/ast.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 12acb6967c25..28bf1b560ca3 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -360,6 +360,10 @@ export function hasDefaultExport(ast: AST): boolean { * * - `"...restElemet1"` --> `"restElement1"` * - `"...restElemet2"` --> `"restElement2"` + * + * DISCLAIMER: This function only correcly extracts the name of `RestElements` in the context of export statements. + * Using this for `RestElements` outside of exports would require us to handle more edgecases. Hence the "Export" in + * this function's name. */ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { // This function returns an array instead of `string | undefined` for convenience since we exclusively use the return @@ -367,6 +371,8 @@ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement) return Identifier.check(restElement.argument) ? [restElement.argument.name] : []; } +const asdf = { ...({ a: 1 } || { b: 2 }) }; + /** * Extracts all identifier names (`'constName'`) from an destructuringassignment'sArrayPattern (the `[constName]` in`const [constName] = [1]`). * @@ -383,6 +389,10 @@ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement) * `[{ foo: name1 }, [{ bar: [name2]}, name3]]` * * Applying this function to this `ArrayPattern` will return the following: `["name1", "name2", "name3"]` + * + * DISCLAIMER: This function only correcly extracts identifiers of `ArrayPatterns` in the context of export statements. + * Using this for `ArrayPattern` outside of exports would require us to handle more edgecases. Hence the "Export" in + * this function's name. */ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPattern): string[] { const identifiers: string[] = []; @@ -412,6 +422,10 @@ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPatte * Example: * export const { foo: [name1], bar: { baz: [{ quux: name2 }], ...name3 }} = { foo: [1], bar: { baz: [{ quux: 3 }]} }; * --> ["name1", "name2", "name3"] + * + * DISCLAIMER: This function only correcly extracts identifiers of `ObjectPatterns` in the context of export statements. + * Using this for `ObjectPatterns` outside of exports would require us to handle more edgecases. Hence the "Export" in + * this function's name. */ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.ObjectPattern): string[] { const identifiers: string[] = []; From b7fc5a61efab0e71278641271b17c3250dc3c545 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 10 Aug 2022 16:01:00 +0000 Subject: [PATCH 24/25] Explain `getExportIdentifiersFromObjectPattern` --- packages/nextjs/src/config/loaders/ast.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index 28bf1b560ca3..be9456dfb113 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -371,8 +371,6 @@ function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement) return Identifier.check(restElement.argument) ? [restElement.argument.name] : []; } -const asdf = { ...({ a: 1 } || { b: 2 }) }; - /** * Extracts all identifier names (`'constName'`) from an destructuringassignment'sArrayPattern (the `[constName]` in`const [constName] = [1]`). * @@ -414,14 +412,21 @@ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPatte /** * Grabs all identifiers from an ObjectPattern within a destructured named export declaration - * statement ("export const { val: name } = { val: 1 }"). + * statement (`name` in "export const { val: name } = { val: 1 }"). * * This function recursively calls itself and `getExportIdentifiersFromArrayPattern` since destructuring assignments * can be deeply nested with objects and arrays. * - * Example: - * export const { foo: [name1], bar: { baz: [{ quux: name2 }], ...name3 }} = { foo: [1], bar: { baz: [{ quux: 3 }]} }; - * --> ["name1", "name2", "name3"] + * Example - take the following program: + * + * ```js + * export const { foo: [{ bar: name1 }], name2, ...name3 } = { foo: [{}] }; + * ``` + * + * The `ObjectPattern` node in question for this program is the left hand side of the assignment: + * `{ foo: [{ bar: name1 }], name2, ...name3 } = { foo: [{}] }` + * + * Applying this function to this `ObjectPattern` will return the following: `["name1", "name2", "name3"]` * * DISCLAIMER: This function only correcly extracts identifiers of `ObjectPatterns` in the context of export statements. * Using this for `ObjectPatterns` outside of exports would require us to handle more edgecases. Hence the "Export" in From 62bba32f44860589700c933a3b48cdbe0bf373ea Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 11 Aug 2022 08:16:25 +0000 Subject: [PATCH 25/25] Remove unnecessary `getIdentifierFromRestElement` --- packages/nextjs/src/config/loaders/ast.ts | 43 ++++++----------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/packages/nextjs/src/config/loaders/ast.ts b/packages/nextjs/src/config/loaders/ast.ts index be9456dfb113..b2e0b62b7afe 100644 --- a/packages/nextjs/src/config/loaders/ast.ts +++ b/packages/nextjs/src/config/loaders/ast.ts @@ -345,32 +345,6 @@ export function hasDefaultExport(ast: AST): boolean { return defaultExports.length > 0; } -/** - * Extracts all identifiers from a `RestElement` within a named export declaration statement (`export const constName = constValue;`). - * `RestElements` are things like `...others` inside a destructuring assignment. - * - * Example - take the following program: - * - * ```js - * export const { bar: { ...restElement1 }, ...restElement2 } = { foo: { baz: 1 }, bar: 2 }; - * ``` - * - * The `RestElement` nodes in this program are "...restElement1" and "...restElement2". This function takes such nodes - * and gets their identifier names. - * - * - `"...restElemet1"` --> `"restElement1"` - * - `"...restElemet2"` --> `"restElement2"` - * - * DISCLAIMER: This function only correcly extracts the name of `RestElements` in the context of export statements. - * Using this for `RestElements` outside of exports would require us to handle more edgecases. Hence the "Export" in - * this function's name. - */ -function getExportIdentifiersFromRestElement(restElement: jscsTypes.RestElement): string[] { - // This function returns an array instead of `string | undefined` for convenience since we exclusively use the return - // value with `array.push()` in other functions in this file. - return Identifier.check(restElement.argument) ? [restElement.argument.name] : []; -} - /** * Extracts all identifier names (`'constName'`) from an destructuringassignment'sArrayPattern (the `[constName]` in`const [constName] = [1]`). * @@ -402,8 +376,9 @@ function getExportIdentifiersFromArrayPattern(arrayPattern: jscsTypes.ArrayPatte identifiers.push(...getExportIdentifiersFromObjectPattern(element)); } else if (ArrayPattern.check(element)) { identifiers.push(...getExportIdentifiersFromArrayPattern(element)); - } else if (RestElement.check(element)) { - identifiers.push(...getExportIdentifiersFromRestElement(element)); + } else if (RestElement.check(element) && Identifier.check(element.argument)) { + // `RestElements` are spread operators + identifiers.push(element.argument.name); } }); @@ -444,11 +419,15 @@ function getExportIdentifiersFromObjectPattern(objectPatternNode: jscsTypes.Obje identifiers.push(...getExportIdentifiersFromObjectPattern(property.value)); } else if (ArrayPattern.check(property.value)) { identifiers.push(...getExportIdentifiersFromArrayPattern(property.value)); - } else if (RestElement.check(property.value)) { - identifiers.push(...getExportIdentifiersFromRestElement(property.value)); + } else if (RestElement.check(property.value) && Identifier.check(property.value.argument)) { + // `RestElements` are spread operators + identifiers.push(property.value.argument.name); } - } else if (RestElement.check(property)) { - identifiers.push(...getExportIdentifiersFromRestElement(property)); + // @ts-ignore AST types are wrong here + } else if (RestElement.check(property) && Identifier.check(property.argument)) { + // `RestElements` are spread operators + // @ts-ignore AST types are wrong here + identifiers.push(property.argument.name as string); } });