From 0a7cd3c763a6492f569bfd2506c4e8c71b35ad03 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Mon, 14 Oct 2024 12:47:15 +0200 Subject: [PATCH] review comments --- packages/nuxt/src/vite/addServerConfig.ts | 38 ++++++++++++++--------- packages/nuxt/src/vite/utils.ts | 16 ++++++---- packages/nuxt/test/vite/utils.test.ts | 22 +++++++------ 3 files changed, 45 insertions(+), 31 deletions(-) diff --git a/packages/nuxt/src/vite/addServerConfig.ts b/packages/nuxt/src/vite/addServerConfig.ts index 1ac7715e608c..5a5d2e5bd627 100644 --- a/packages/nuxt/src/vite/addServerConfig.ts +++ b/packages/nuxt/src/vite/addServerConfig.ts @@ -10,7 +10,7 @@ import { SENTRY_FUNCTIONS_REEXPORT, SENTRY_WRAPPED_ENTRY, constructFunctionReExport, - stripQueryPart, + removeSentryQueryFromPath, } from './utils'; const SERVER_CONFIG_FILENAME = 'sentry.server.config'; @@ -147,11 +147,12 @@ export function addDynamicImportEntryFileWrapper(nitro: Nitro, serverConfigFile: ); } +/** + * A Rollup plugin which wraps the server entry with a dynamic `import()`. This makes it possible to initialize Sentry first + * by using a regular `import` and load the server after that. + * This also works with serverless `handler` functions, as it re-exports the `handler`. + */ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPluginOption { - const containsSuffix = (sourcePath: string): boolean => { - return sourcePath.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`) || sourcePath.includes(SENTRY_FUNCTIONS_REEXPORT); - }; - return { name: 'sentry-wrap-entry-with-dynamic-import', async resolveId(source, importer, options) { @@ -160,27 +161,31 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug } if (source === 'import-in-the-middle/hook.mjs') { + // We are importing "import-in-the-middle" in the returned code of the `load()` function below + // By setting `moduleSideEffects` to `true`, the import is added to the bundle, although nothing is imported from it + // By importing "import-in-the-middle/hook.mjs", we can make sure this file is included, as not all node builders are including files imported with `module.register()`. + // Prevents the error "Failed to register ESM hook Error: Cannot find module 'import-in-the-middle/hook.mjs'" return { id: source, moduleSideEffects: true, external: true }; } - if (options.isEntry && !source.includes(SENTRY_WRAPPED_ENTRY)) { + if (options.isEntry && !source.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`)) { const resolution = await this.resolve(source, importer, options); - // If it cannot be resolved or is external, just return it - // so that Rollup can display an error + // If it cannot be resolved or is external, just return it so that Rollup can display an error if (!resolution || resolution?.external) return resolution; const moduleInfo = await this.load(resolution); moduleInfo.moduleSideEffects = true; + // The key `.` in `exportedBindings` refer to the exports within the file const exportedFunctions = moduleInfo.exportedBindings?.['.']; - // checks are needed to prevent multiple attachment of the suffix - return containsSuffix(source) || containsSuffix(resolution.id) + // The enclosing `if` already checks for the suffix in `source`, but a check in `resolution.id` is needed as well to prevent multiple attachment of the suffix + return resolution.id.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`) ? resolution.id : resolution.id - // concat the query params to mark the file (also attaches names of exports - this is needed for serverless functions to re-export the handler) + // Concatenates the query params to mark the file (also attaches names of re-exports - this is needed for serverless functions to re-export the handler) .concat(SENTRY_WRAPPED_ENTRY) .concat( exportedFunctions?.length @@ -192,18 +197,21 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug }, load(id: string) { if (id.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`)) { - const entryId = stripQueryPart(id); + const entryId = removeSentryQueryFromPath(id); + // Mostly useful for serverless `handler` functions const reExportedFunctions = id.includes(SENTRY_FUNCTIONS_REEXPORT) ? constructFunctionReExport(id, entryId) : ''; return ( - // Import the Sentry server config + // Regular `import` of the Sentry config `import ${JSON.stringify(resolvedSentryConfigPath)};\n` + - // Dynamic import for the previous, actual entry point. - // import() can be used for any code that should be run after the hooks are registered (https://nodejs.org/api/module.html#enabling) + // Dynamic `import()` for the previous, actual entry point. + // `import()` can be used for any code that should be run after the hooks are registered (https://nodejs.org/api/module.html#enabling) `import(${JSON.stringify(entryId)});\n` + + // By importing "import-in-the-middle/hook.mjs", we can make sure this file wil be included, as not all node builders are including files imported with `module.register()`. + "import 'import-in-the-middle/hook.mjs'\n" + `${reExportedFunctions}\n` ); } diff --git a/packages/nuxt/src/vite/utils.ts b/packages/nuxt/src/vite/utils.ts index 32318cf8a8a8..4d5bc080ba3a 100644 --- a/packages/nuxt/src/vite/utils.ts +++ b/packages/nuxt/src/vite/utils.ts @@ -30,18 +30,21 @@ export const SENTRY_FUNCTIONS_REEXPORT = '?sentry-query-functions-reexport='; export const QUERY_END_INDICATOR = 'SENTRY-QUERY-END'; /** - * Strips a specific query part from a URL. + * Strips the Sentry query part from a path. + * Example: example/path?sentry-query-wrapped-entry?sentry-query-functions-reexport=foo,SENTRY-QUERY-END -> /example/path * * Only exported for testing. */ -export function stripQueryPart(url: string): string { +export function removeSentryQueryFromPath(url: string): string { // eslint-disable-next-line @sentry-internal/sdk/no-regexp-constructor const regex = new RegExp(`\\${SENTRY_WRAPPED_ENTRY}.*?\\${QUERY_END_INDICATOR}`); return url.replace(regex, ''); } /** - * Extracts and sanitizes function reexport query parameters from a query string. + * Extracts and sanitizes function re-export query parameters from a query string. + * If it is a default export, it is not considered for re-exporting. This function is mostly relevant for re-exporting + * serverless `handler` functions. * * Only exported for testing. */ @@ -55,7 +58,7 @@ export function extractFunctionReexportQueryParameters(query: string): string[] ? match[1] .split(',') .filter(param => param !== '' && param !== 'default') - // sanitize + // Sanitize, as code could be injected with another rollup plugin .map((str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) : []; } @@ -69,8 +72,9 @@ export function constructFunctionReExport(pathWithQuery: string, entryId: string return functionNames.reduce( (functionsCode, currFunctionName) => functionsCode.concat( - `export function ${currFunctionName}(...args) {\n` + - ` return import(${JSON.stringify(entryId)}).then((res) => res.${currFunctionName}(...args));\n` + + `export async function ${currFunctionName}(...args) {\n` + + ` const res = await import(${JSON.stringify(entryId)});\n` + + ` return res.${currFunctionName}.call(this, ...args);\n` + '}\n', ), '', diff --git a/packages/nuxt/test/vite/utils.test.ts b/packages/nuxt/test/vite/utils.test.ts index f29c2f5f2bcc..0d7e91b8b83f 100644 --- a/packages/nuxt/test/vite/utils.test.ts +++ b/packages/nuxt/test/vite/utils.test.ts @@ -7,7 +7,7 @@ import { constructFunctionReExport, extractFunctionReexportQueryParameters, findDefaultSdkInitFile, - stripQueryPart, + removeSentryQueryFromPath, } from '../../src/vite/utils'; vi.mock('fs'); @@ -68,16 +68,16 @@ describe('findDefaultSdkInitFile', () => { }); }); -describe('stripQueryPart', () => { - it('strips the specific query part from the URL', () => { +describe('removeSentryQueryFromPath', () => { + it('strips the Sentry query part from the path', () => { const url = `/example/path${SENTRY_WRAPPED_ENTRY}${SENTRY_FUNCTIONS_REEXPORT}foo,${QUERY_END_INDICATOR}`; - const result = stripQueryPart(url); + const result = removeSentryQueryFromPath(url); expect(result).toBe('/example/path'); }); - it('returns the same URL if the specific query part is not present', () => { + it('returns the same path if the specific query part is not present', () => { const url = '/example/path?other-query=param'; - const result = stripQueryPart(url); + const result = removeSentryQueryFromPath(url); expect(result).toBe(url); }); }); @@ -108,11 +108,13 @@ describe('constructFunctionReExport', () => { const result2 = constructFunctionReExport(query2, entryId); const expected = ` -export function foo(...args) { - return import("./module").then((res) => res.foo(...args)); +export async function foo(...args) { + const res = await import("./module"); + return res.foo.call(this, ...args); } -export function bar(...args) { - return import("./module").then((res) => res.bar(...args)); +export async function bar(...args) { + const res = await import("./module"); + return res.bar.call(this, ...args); }`; expect(result.trim()).toBe(expected.trim()); expect(result2.trim()).toBe(expected.trim());