From 66fa4422d5b6f278e4b8579bbb5bc01bf66d7dfa Mon Sep 17 00:00:00 2001 From: AlbertoMihai98 Date: Fri, 4 Sep 2026 00:55:01 +0300 Subject: [PATCH] fix(nextjs): Lazy-load the Pages Router module so App Router apps do not bundle the Pages Router runtime `client/routing/pagesRouterRoutingInstrumentation.ts` imported `next/router` statically and resolved its CJS/ESM interop at module scope. That module is the whole Pages Router client runtime (the `Router` class, path-to-regexp, the route loader, script.js, ...), it is reached statically from the client entry, and whether an app uses the Pages Router is only known at runtime - so every App Router app shipped ~87 KB raw / ~36 KB gzip of code it can never execute, and no bundler could remove it (`next` declares no `sideEffects`). The router is now imported on demand, only from `pagesRouterInstrumentNavigation`, the single place that needs it; the pageload instrumentation is unchanged and still synchronous. The import targets `next/dist/client/router` rather than the `next/router` shim: the shim is not part of a Pages Router app's initial chunks and became a tiny extra chunk request on every pageload (151 bytes on Turbopack, 99 on webpack), while the module itself is already loaded by the framework runtime, so importing it directly adds no request. Tests: the navigation tests await `vi.dynamicImportSettled()`; a new test pins that the listener is registered exactly once and only after the import settles, and that the pageload path never touches the router; `test/clientEntryBundlerGraph.test.ts` requires the built CJS client entry in a child process and fails if `next/router` or `next/dist/client/router` is in the module cache (with a positive control on the instrumentation module itself). Measured on an App Router app (Next 16.3.3, Turbopack): Sentry client chunk 168.4 KB -> 82 KB raw; the Pages Router runtime lands in an async chunk the app never requests. Co-Authored-By: Claude Fable 5.1 --- .../pagesRouterRoutingInstrumentation.ts | 106 ++++++++++++------ .../test/clientEntryBundlerGraph.test.ts | 40 +++++++ .../pagesRouterInstrumentation.test.ts | 40 ++++++- 3 files changed, 144 insertions(+), 42 deletions(-) create mode 100755 packages/nextjs/test/clientEntryBundlerGraph.test.ts 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)); + }); });