diff --git a/packages/bundler-plugin-core/src/index.ts b/packages/bundler-plugin-core/src/index.ts index a59eeca0..bc350184 100644 --- a/packages/bundler-plugin-core/src/index.ts +++ b/packages/bundler-plugin-core/src/index.ts @@ -206,44 +206,80 @@ type RenderChunkHook = ( code: string, chunk: { fileName: string; + facadeModuleId?: string | null; } ) => { code: string; map: SourceMap; } | null; +/** + * Checks if a file is a JavaScript file based on its extension. + * Handles query strings and hashes in the filename. + */ +function isJsFile(fileName: string): boolean { + const cleanFileName = stripQueryAndHashFromPath(fileName); + return [".js", ".mjs", ".cjs"].some((ext) => cleanFileName.endsWith(ext)); +} + +/** + * Checks if a chunk should be skipped for code injection + * + * This is necessary to handle Vite's MPA (multi-page application) mode where + * HTML entry points create "facade" chunks that should not contain injected code. + * See: https://github.com/getsentry/sentry-javascript-bundler-plugins/issues/829 + * + * @param code - The chunk's code content + * @param facadeModuleId - The facade module ID (if any) - HTML files create facade chunks + * @returns true if the chunk should be skipped + */ +function shouldSkipCodeInjection(code: string, facadeModuleId: string | null | undefined): boolean { + // Skip empty chunks - these are placeholder chunks that should be optimized away + if (code.trim().length === 0) { + return true; + } + + // Skip HTML facade chunks + // They only contain import statements and should not have Sentry code injected + if (facadeModuleId && stripQueryAndHashFromPath(facadeModuleId).endsWith(".html")) { + return true; + } + + return false; +} + export function createRollupReleaseInjectionHooks(injectionCode: string): { renderChunk: RenderChunkHook; } { return { - renderChunk(code: string, chunk: { fileName: string }) { - if ( - // chunks could be any file (html, md, ...) - [".js", ".mjs", ".cjs"].some((ending) => - stripQueryAndHashFromPath(chunk.fileName).endsWith(ending) - ) - ) { - const ms = new MagicString(code, { filename: chunk.fileName }); + renderChunk(code: string, chunk: { fileName: string; facadeModuleId?: string | null }) { + if (!isJsFile(chunk.fileName)) { + return null; // returning null means not modifying the chunk at all + } - const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; + // Skip empty chunks and HTML facade chunks (Vite MPA) + if (shouldSkipCodeInjection(code, chunk.facadeModuleId)) { + return null; + } - if (match) { - // Add injected code after any comments or "use strict" at the beginning of the bundle. - ms.appendLeft(match.length, injectionCode); - } else { - // ms.replace() doesn't work when there is an empty string match (which happens if - // there is neither, a comment, nor a "use strict" at the top of the chunk) so we - // need this special case here. - ms.prepend(injectionCode); - } + const ms = new MagicString(code, { filename: chunk.fileName }); - return { - code: ms.toString(), - map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), - }; + const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; + + if (match) { + // Add injected code after any comments or "use strict" at the beginning of the bundle. + ms.appendLeft(match.length, injectionCode); } else { - return null; // returning null means not modifying the chunk at all + // ms.replace() doesn't work when there is an empty string match (which happens if + // there is neither, a comment, nor a "use strict" at the top of the chunk) so we + // need this special case here. + ms.prepend(injectionCode); } + + return { + code: ms.toString(), + map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), + }; }, }; } @@ -262,48 +298,48 @@ export function createRollupDebugIdInjectionHooks(): { renderChunk: RenderChunkHook; } { return { - renderChunk(code: string, chunk: { fileName: string }) { + renderChunk(code: string, chunk: { fileName: string; facadeModuleId?: string | null }) { + if (!isJsFile(chunk.fileName)) { + return null; // returning null means not modifying the chunk at all + } + + // Skip empty chunks and HTML facade chunks (Vite MPA) + if (shouldSkipCodeInjection(code, chunk.facadeModuleId)) { + return null; + } + + // Check if a debug ID has already been injected to avoid duplicate injection (e.g. by another plugin or Sentry CLI) + const chunkStartSnippet = code.slice(0, 6000); + const chunkEndSnippet = code.slice(-500); + if ( - // chunks could be any file (html, md, ...) - [".js", ".mjs", ".cjs"].some((ending) => - stripQueryAndHashFromPath(chunk.fileName).endsWith(ending) - ) + chunkStartSnippet.includes("_sentryDebugIdIdentifier") || + chunkEndSnippet.includes("//# debugId=") ) { - // Check if a debug ID has already been injected to avoid duplicate injection (e.g. by another plugin or Sentry CLI) - const chunkStartSnippet = code.slice(0, 6000); - const chunkEndSnippet = code.slice(-500); - - if ( - chunkStartSnippet.includes("_sentryDebugIdIdentifier") || - chunkEndSnippet.includes("//# debugId=") - ) { - return null; // Debug ID already present, skip injection - } - - const debugId = stringToUUID(code); // generate a deterministic debug ID - const codeToInject = getDebugIdSnippet(debugId); + return null; // Debug ID already present, skip injection + } - const ms = new MagicString(code, { filename: chunk.fileName }); + const debugId = stringToUUID(code); // generate a deterministic debug ID + const codeToInject = getDebugIdSnippet(debugId); - const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; + const ms = new MagicString(code, { filename: chunk.fileName }); - if (match) { - // Add injected code after any comments or "use strict" at the beginning of the bundle. - ms.appendLeft(match.length, codeToInject); - } else { - // ms.replace() doesn't work when there is an empty string match (which happens if - // there is neither, a comment, nor a "use strict" at the top of the chunk) so we - // need this special case here. - ms.prepend(codeToInject); - } + const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; - return { - code: ms.toString(), - map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), - }; + if (match) { + // Add injected code after any comments or "use strict" at the beginning of the bundle. + ms.appendLeft(match.length, codeToInject); } else { - return null; // returning null means not modifying the chunk at all + // ms.replace() doesn't work when there is an empty string match (which happens if + // there is neither, a comment, nor a "use strict" at the top of the chunk) so we + // need this special case here. + ms.prepend(codeToInject); } + + return { + code: ms.toString(), + map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), + }; }, }; } @@ -312,34 +348,34 @@ export function createRollupModuleMetadataInjectionHooks(injectionCode: string): renderChunk: RenderChunkHook; } { return { - renderChunk(code: string, chunk: { fileName: string }) { - if ( - // chunks could be any file (html, md, ...) - [".js", ".mjs", ".cjs"].some((ending) => - stripQueryAndHashFromPath(chunk.fileName).endsWith(ending) - ) - ) { - const ms = new MagicString(code, { filename: chunk.fileName }); + renderChunk(code: string, chunk: { fileName: string; facadeModuleId?: string | null }) { + if (!isJsFile(chunk.fileName)) { + return null; // returning null means not modifying the chunk at all + } - const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; + // Skip empty chunks and HTML facade chunks (Vite MPA) + if (shouldSkipCodeInjection(code, chunk.facadeModuleId)) { + return null; + } - if (match) { - // Add injected code after any comments or "use strict" at the beginning of the bundle. - ms.appendLeft(match.length, injectionCode); - } else { - // ms.replace() doesn't work when there is an empty string match (which happens if - // there is neither, a comment, nor a "use strict" at the top of the chunk) so we - // need this special case here. - ms.prepend(injectionCode); - } + const ms = new MagicString(code, { filename: chunk.fileName }); - return { - code: ms.toString(), - map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), - }; + const match = code.match(COMMENT_USE_STRICT_REGEX)?.[0]; + + if (match) { + // Add injected code after any comments or "use strict" at the beginning of the bundle. + ms.appendLeft(match.length, injectionCode); } else { - return null; // returning null means not modifying the chunk at all + // ms.replace() doesn't work when there is an empty string match (which happens if + // there is neither, a comment, nor a "use strict" at the top of the chunk) so we + // need this special case here. + ms.prepend(injectionCode); } + + return { + code: ms.toString(), + map: ms.generateMap({ file: chunk.fileName, hires: "boundary" }), + }; }, }; } diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-with-plugin.ts b/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-with-plugin.ts new file mode 100644 index 00000000..fc9b426d --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-with-plugin.ts @@ -0,0 +1,28 @@ +import { sentryVitePlugin } from "@sentry/vite-plugin"; +import * as path from "path"; +import * as vite from "vite"; + +const inputDir = path.join(__dirname, "input"); + +void vite.build({ + clearScreen: false, + root: inputDir, + build: { + sourcemap: true, + outDir: path.join(__dirname, "out", "with-plugin"), + emptyOutDir: true, + rollupOptions: { + input: { + index: path.join(inputDir, "index.html"), + page1: path.join(inputDir, "page1.html"), + page2: path.join(inputDir, "page2.html"), + }, + }, + }, + plugins: [ + sentryVitePlugin({ + telemetry: false, + // Empty options - the issue says options don't affect the results + }), + ], +}); diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-without-plugin.ts b/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-without-plugin.ts new file mode 100644 index 00000000..85112284 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/build-vite-without-plugin.ts @@ -0,0 +1,22 @@ +import * as path from "path"; +import * as vite from "vite"; + +const inputDir = path.join(__dirname, "input"); + +void vite.build({ + clearScreen: false, + root: inputDir, + build: { + sourcemap: true, + outDir: path.join(__dirname, "out", "without-plugin"), + emptyOutDir: true, + rollupOptions: { + input: { + index: path.join(inputDir, "index.html"), + page1: path.join(inputDir, "page1.html"), + page2: path.join(inputDir, "page2.html"), + }, + }, + }, + plugins: [], +}); diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/index.html b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/index.html new file mode 100644 index 00000000..ae984747 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/index.html @@ -0,0 +1,11 @@ + + + + + Index Page + + +

Index Page - No Scripts

+ + + diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page1.html b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page1.html new file mode 100644 index 00000000..7c8bbff0 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page1.html @@ -0,0 +1,11 @@ + + + + + Page 1 + + +

Page 1 - With Shared Module

+ + + diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page2.html b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page2.html new file mode 100644 index 00000000..d37e9908 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/page2.html @@ -0,0 +1,11 @@ + + + + + Page 2 + + +

Page 2 - With Shared Module

+ + + diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/shared-module.js b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/shared-module.js new file mode 100644 index 00000000..07182654 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/input/shared-module.js @@ -0,0 +1,10 @@ +// This is a shared module that is used by multiple HTML pages +export function greet(name) { + // eslint-disable-next-line no-console + console.log(`Hello, ${String(name)}!`); +} + +export const VERSION = "1.0.0"; + +// Side effect: greet on load +greet("World"); diff --git a/packages/integration-tests/fixtures/vite-mpa-extra-modules/vite-mpa-extra-modules.test.ts b/packages/integration-tests/fixtures/vite-mpa-extra-modules/vite-mpa-extra-modules.test.ts new file mode 100644 index 00000000..6e6bbc56 --- /dev/null +++ b/packages/integration-tests/fixtures/vite-mpa-extra-modules/vite-mpa-extra-modules.test.ts @@ -0,0 +1,103 @@ +/** + * Test for GitHub Issue #829: + * sentryVitePlugin creates unnecessary JS modules for each index page + * + * In a Vite multi-page app (MPA), sentryVitePlugin causes "vite build" to emit + * a separate, unique, unexpected JS module for each index page. + * + * Expected: The plugin should add metadata to generated modules but should NOT + * rework the import graph, create new modules, or add script tags to pages + * that didn't have them in the first place. + * + * Actual: A unique module is generated for every HTML page in rollup.options.input. + * Pages that had been sharing modules will instead load a page-specific module + * that imports the shared code. Pages that didn't contain ANY scripts will now have one. + */ +import childProcess from "child_process"; +import fs from "fs"; +import path from "path"; + +function getAssetFiles(outDir: string): string[] { + const assetsDir = path.join(outDir, "assets"); + if (!fs.existsSync(assetsDir)) { + return []; + } + return fs.readdirSync(assetsDir).filter((file) => file.endsWith(".js")); +} + +function getScriptTagsFromHtml(htmlPath: string): string[] { + if (!fs.existsSync(htmlPath)) { + return []; + } + const content = fs.readFileSync(htmlPath, "utf-8"); + const scriptMatches = content.match(/]*>/g) || []; + return scriptMatches; +} + +describe("Vite MPA Extra Modules Issue (#829)", () => { + const outWithoutPlugin = path.join(__dirname, "out", "without-plugin"); + const outWithPlugin = path.join(__dirname, "out", "with-plugin"); + + beforeAll(() => { + // Clean output directories + if (fs.existsSync(outWithoutPlugin)) { + fs.rmSync(outWithoutPlugin, { recursive: true }); + } + if (fs.existsSync(outWithPlugin)) { + fs.rmSync(outWithPlugin, { recursive: true }); + } + + // Build without plugin + childProcess.execSync(`yarn ts-node ${path.join(__dirname, "build-vite-without-plugin.ts")}`, { + encoding: "utf-8", + stdio: "inherit", + }); + + // Build with plugin + childProcess.execSync(`yarn ts-node ${path.join(__dirname, "build-vite-with-plugin.ts")}`, { + encoding: "utf-8", + stdio: "inherit", + }); + }, 60_000); + + it("should not create extra JS modules when sentry plugin is enabled", () => { + const assetsWithoutPlugin = getAssetFiles(outWithoutPlugin); + const assetsWithPlugin = getAssetFiles(outWithPlugin); + + // The number of JS files should be the same (or very close) + // With the bug, the plugin creates extra modules like index.js, page1.js, page2.js + // for each HTML entry point + expect(assetsWithPlugin).toHaveLength(assetsWithoutPlugin.length); + }); + + it("should not add script tags to HTML pages that had none", () => { + // index.html originally has no scripts + const indexWithoutPlugin = getScriptTagsFromHtml(path.join(outWithoutPlugin, "index.html")); + const indexWithPlugin = getScriptTagsFromHtml(path.join(outWithPlugin, "index.html")); + + // The number of script tags should be the same + // With the bug, index.html gets a script tag added even though it had none + expect(indexWithPlugin).toHaveLength(indexWithoutPlugin.length); + }); + + it("should preserve shared module imports without creating page-specific wrappers", () => { + // page1.html and page2.html should both reference the same shared module + // not page-specific modules + const page1WithPlugin = fs.readFileSync(path.join(outWithPlugin, "page1.html"), "utf-8"); + const page2WithPlugin = fs.readFileSync(path.join(outWithPlugin, "page2.html"), "utf-8"); + + // Extract the JS file references + const page1ScriptMatch = page1WithPlugin.match(/src="([^"]+\.js)"/); + const page2ScriptMatch = page2WithPlugin.match(/src="([^"]+\.js)"/); + + // Both pages should reference the same shared module, not page-specific ones + // With the bug, page1.html references page1-xxx.js and page2.html references page2-xxx.js + // instead of both referencing shared-module-xxx.js + const page1Script = page1ScriptMatch?.[1]; + const page2Script = page2ScriptMatch?.[1]; + + // They should NOT be page-specific (named after the page) + expect(page1Script).not.toMatch(/page1/i); + expect(page2Script).not.toMatch(/page2/i); + }); +});