diff --git a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts index 1aee7a2e0c58..562138ded28f 100644 --- a/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/pagesRouterRoutingInstrumentation.ts @@ -15,18 +15,41 @@ import { WINDOW, } from '@sentry/react'; import type { NEXT_DATA } from 'next/dist/shared/lib/utils'; -import RouterImport from 'next/router'; +import type RouterImport from 'next/router'; import type { ParsedUrlQuery } from 'querystring'; import { DEBUG_BUILD } from '../../common/debug-build'; import { SENTRY_OP, SENTRY_SEGMENT_NAME_SOURCE, URL_TEMPLATE } from '@sentry/conventions/attributes'; import { NAVIGATION, PAGELOAD } from '@sentry/conventions/op'; -// next/router v10 is CJS -// -// For ESM/CJS interoperability 'reasons', depending on how this file is loaded, Router might be on the default export -const Router: typeof RouterImport = RouterImport.events - ? RouterImport - : (RouterImport as unknown as { default: typeof RouterImport }).default; +type NextRouter = typeof RouterImport; + +/** + * Loads the Pages Router singleton (what `next/router` exports) on demand. + * + * It must not be imported statically: it is the whole Pages Router client runtime (the `Router` class, + * `path-to-regexp`, the route loader, ...), `next` does not declare `sideEffects`, and whether an app uses + * the Pages Router is only known at runtime (see `nextRoutingInstrumentation.ts`). A static import therefore + * lands the entire Pages Router in the client bundle of every app - App Router apps included, which never + * reach this code - and no bundler can tree-shake it away, with or without `__SENTRY_TRACING__`. Behind + * `import()` the module stays out of the initial graph. + * + * `next/dist/client/router` rather than the public `next/router` entry on purpose: that entry is a one-line + * CJS shim re-exporting this module, and a shim that is not in the app's initial chunks becomes a tiny extra + * chunk request on every Pages Router pageload (151 bytes on Turbopack, 99 on webpack when measured). The + * module itself is already part of the Pages Router runtime, so importing it directly adds no request and + * resolves on the next microtask. The type still comes from `next/router`; it is the same object. + */ +function loadNextRouter(): Promise { + return import('next/dist/client/router').then(routerModule => { + // next/router v10 is CJS + // + // For ESM/CJS interoperability 'reasons', depending on how this file is loaded, Router might be the + // namespace itself, sit on its default export, or on the default export's default export. + const namespace = routerModule as unknown as { default?: NextRouter }; + const candidate = (namespace.default ?? namespace) as NextRouter; + return candidate.events ? candidate : (candidate as unknown as { default: NextRouter }).default; + }); +} const globalObject = WINDOW; @@ -144,39 +167,48 @@ export function pagesRouterInstrumentPageLoad(client: Client): void { * * Leverages the SingletonRouter from the `next/router` to * generate pageload/navigation transactions and parameterize - * transaction names. + * transaction names. The router is loaded on demand (see `loadNextRouter`), so the + * `routeChangeStart` listener is registered once that import has settled. */ export function pagesRouterInstrumentNavigation(client: Client): void { - Router.events.on('routeChangeStart', (navigationTarget: string) => { - const strippedNavigationTarget = stripUrlQueryAndFragment(navigationTarget); - const matchedRoute = getNextRouteFromPathname(strippedNavigationTarget); - - let newLocation: string; - let spanSource: TransactionSource; - - if (matchedRoute) { - newLocation = matchedRoute; - spanSource = 'route'; - } else { - newLocation = strippedNavigationTarget; - spanSource = 'url'; - } + void loadNextRouter() + .then(Router => { + Router.events.on('routeChangeStart', (navigationTarget: string) => { + const strippedNavigationTarget = stripUrlQueryAndFragment(navigationTarget); + const matchedRoute = getNextRouteFromPathname(strippedNavigationTarget); - startBrowserTracingNavigationSpan( - client, - { - // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. - name: spanSource === 'route' || !hasSpanStreamingEnabled(client) ? newLocation : NAVIGATION_SPAN_NAME_FALLBACK, - attributes: { - [SENTRY_OP]: NAVIGATION, - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.pages_router_instrumentation', - [SENTRY_SEGMENT_NAME_SOURCE]: spanSource, - ...(spanSource === 'route' && { [URL_TEMPLATE]: newLocation }), - }, - }, - { url: getAbsoluteUrl(navigationTarget) }, - ); - }); + let newLocation: string; + let spanSource: TransactionSource; + + if (matchedRoute) { + newLocation = matchedRoute; + spanSource = 'route'; + } else { + newLocation = strippedNavigationTarget; + spanSource = 'url'; + } + + startBrowserTracingNavigationSpan( + client, + { + // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. + name: + spanSource === 'route' || !hasSpanStreamingEnabled(client) ? newLocation : NAVIGATION_SPAN_NAME_FALLBACK, + attributes: { + [SENTRY_OP]: NAVIGATION, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.pages_router_instrumentation', + [SENTRY_SEGMENT_NAME_SOURCE]: spanSource, + ...(spanSource === 'route' && { [URL_TEMPLATE]: newLocation }), + }, + }, + { url: getAbsoluteUrl(navigationTarget) }, + ); + }); + }) + .catch((error: unknown) => { + DEBUG_BUILD && + debug.warn('Could not load `next/router`, Pages Router navigations will not be instrumented:', error); + }); } function getNextRouteFromPathname(pathname: string): string | undefined { diff --git a/packages/nextjs/test/clientEntryBundlerGraph.test.ts b/packages/nextjs/test/clientEntryBundlerGraph.test.ts new file mode 100755 index 000000000000..4d228f63df51 --- /dev/null +++ b/packages/nextjs/test/clientEntryBundlerGraph.test.ts @@ -0,0 +1,40 @@ +import { spawnSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Importing the SDK client entry must not load `next/router`. That module is the whole Pages Router client + * runtime, and a static import of it lands in the client bundle of every app - App Router apps included, + * which never reach the Pages Router branch of the routing instrumentation. Bundlers cannot remove it + * (`next` declares no `sideEffects`, and the app/pages decision is made at runtime), so the durable guard + * is that the entry's module graph does not contain it: `pagesRouterRoutingInstrumentation` imports the + * router on demand instead. Runs in a child process for a clean module cache and real Node resolution, + * like `serverEntryBundlerGraph.test.ts`. + */ +describe('built CJS client entry', () => { + const clientEntry = resolve(__dirname, '../build/cjs/client/index.js'); + + it('does not load `next/router` at import time', () => { + const script = ` + require(${JSON.stringify(clientEntry)}); + const toPosix = modulePath => modulePath.split(require('path').sep).join('/'); + const loaded = Object.keys(require.cache).map(toPosix); + // Control: the Pages Router instrumentation itself must be in the graph, or an empty list proves nothing. + if (!loaded.some(modulePath => modulePath.endsWith('/client/routing/pagesRouterRoutingInstrumentation.js'))) { + console.error('Control failed: the Pages Router routing instrumentation was not loaded at all'); + process.exit(2); + } + const routerModules = loaded.filter( + modulePath => modulePath.endsWith('/next/router.js') || modulePath.includes('/next/dist/client/router'), + ); + if (routerModules.length > 0) { + console.error('next/router loaded at import time:\\n' + routerModules.join('\\n')); + process.exit(1); + } + `; + + // On failure, stderr carries the leaked module list, the failed control, or the import crash itself. + const result = spawnSync(process.execPath, ['-e', script], { encoding: 'utf8' }); + expect(result.status, result.stderr).toBe(0); + }); +}); diff --git a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts index 356d7d3db092..cdba0cda28e6 100644 --- a/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts +++ b/packages/nextjs/test/performance/pagesRouterInstrumentation.test.ts @@ -2,7 +2,8 @@ import type { Client } from '@sentry/core'; import { WINDOW } from '@sentry/react'; import { JSDOM } from 'jsdom'; import type { NEXT_DATA } from 'next/dist/shared/lib/utils'; -import Router from 'next/router'; +// The instrumentation imports the module behind the `next/router` shim on demand, so that is what is mocked. +import Router from 'next/dist/client/router'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { pagesRouterInstrumentNavigation, @@ -21,17 +22,17 @@ const originalBuildManifestRoutes = globalObject.__BUILD_MANIFEST?.sortedPages; let eventHandlers: { [eventName: string]: Set<(...args: any[]) => void> } = {}; -vi.mock('next/router', () => { +vi.mock('next/dist/client/router', () => { return { default: { events: { - on(type: string, handler: (...args: any[]) => void) { + on: vi.fn((type: string, handler: (...args: any[]) => void) => { if (!eventHandlers[type]) { eventHandlers[type] = new Set(); } eventHandlers[type]!.add(handler); - }, + }), off: vi.fn((type: string, handler: (...args: any[]) => void) => { if (eventHandlers[type]) { eventHandlers[type]!.delete(handler); @@ -300,7 +301,7 @@ describe('pagesRouterInstrumentNavigation', () => { ['/e/f/g', '/e/[f]/[g]/[[...h]]', 'route'], ])( 'should create a parameterized transaction on route change (%s)', - (targetLocation, expectedTransactionName, expectedTransactionSource) => { + async (targetLocation, expectedTransactionName, expectedTransactionSource) => { setUpNextPage({ url: 'https://example.com/home', route: '/home', @@ -325,6 +326,8 @@ describe('pagesRouterInstrumentNavigation', () => { } as unknown as Client; pagesRouterInstrumentNavigation(client); + // The router is imported on demand; the listener exists once that import has settled. + await vi.dynamicImportSettled(); Router.events.emit('routeChangeStart', targetLocation); @@ -352,4 +355,31 @@ describe('pagesRouterInstrumentNavigation', () => { }); }, ); + + it('registers the route change listener only once the on-demand router import has settled', async () => { + setUpNextPage({ + url: 'https://example.com/home', + route: '/home', + hasNextData: true, + navigatableRoutes: ['/home'], + }); + + const client = { + emit: vi.fn(), + getOptions: () => ({}), + } as unknown as Client; + + // The pageload instrumentation reads `__NEXT_DATA__` and the build manifest only - it never needs the router. + pagesRouterInstrumentPageLoad(client); + expect(Router.events.on).not.toHaveBeenCalled(); + + // The navigation instrumentation imports the router on demand: nothing is registered synchronously ... + pagesRouterInstrumentNavigation(client); + expect(Router.events.on).not.toHaveBeenCalled(); + + // ... and exactly one listener once the import has settled. + await vi.dynamicImportSettled(); + expect(Router.events.on).toHaveBeenCalledTimes(1); + expect(Router.events.on).toHaveBeenCalledWith('routeChangeStart', expect.any(Function)); + }); });