fix(pages-router): run instrumentation-client.ts before hydration (#1474)

[james-elicx]

Summary

• Side-effect-import the user's instrumentation-client.{ts,js} from the generated Pages Router client entry, before hydrateRoot(). Previously the contract was implicit (transitive load via private-next-instrumentation-client), which left the user's top-level statements vulnerable to tree-shaking and ordering surprises.
• Mirrors Next.js's page-bootstrap.ts, which side-effect-imports require-instrumentation-client before initialize / hydrate (.nextjs-ref/packages/next/src/client/page-bootstrap.ts L1).
• Adds focused unit coverage in tests/entry-templates.test.ts:
  • The generated client entry imports the user's instrumentation-client.ts and that import precedes hydrateRoot(.
  • Nothing spurious is emitted when no file is present.

Closes #1474 once CI passes.

Test plan

• pnpm test tests/entry-templates.test.ts tests/pages-router.test.ts tests/instrumentation.test.ts — 320 tests pass locally.
• pnpm run check clean.
• CI green (Vitest + Playwright tests/e2e/pages-router/instrumentation-client.spec.ts).

[james-elicx]

/bigbonk review

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1671

commit: 7fe0338

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
web	preview	production

[ask-bonk]

Review: LGTM

Small, focused fix with good test coverage — approving.

What this does

Adds an explicit side-effect import of the user's instrumentation-client.{ts,js} at the very top of the generated Pages Router client entry, before vinext/instrumentation-client and before hydrateRoot(). This matches Next.js's page-bootstrap.ts ordering where require-instrumentation-client is the first import.

What I checked

• Correctness: Bare side-effect import (import "…";) ensures top-level statements execute unconditionally. JSON.stringify + normalizePathSeparators handles path escaping and Windows separators correctly.
• Import ordering: The user file import precedes all other imports including vinext/instrumentation-client, matching Next.js's contract.
• Backward compatibility: Default parameter instrumentationClientPath: string | null = null means existing callers are unaffected.
• Wiring in index.ts: instrumentationClientPath is already resolved at line 1085 via findInstrumentationClientFile() — the one-line change at line 722 is clean plumbing.
• Test coverage: Both the presence case (import appears before hydrateRoot() and absence case (no spurious import when file doesn't exist) are verified. Tests follow the existing tmpDir/try-finally pattern.
• Comment quality: The inline block comment (lines 47-60) explains why the explicit import is needed (tree-shaker safety for indirect loads) and cites the Next.js source.

Nit (non-blocking)

When instrumentationClientPath is null, userInstrumentationImport is "", so the template produces a leading blank line (\nimport "vinext/instrumentation-client";). Cosmetic and harmless for a virtual module, but if you want to clean it up:

const userInstrumentationImport = instrumentationClientPath
    ? `import ${JSON.stringify(normalizePathSeparators(instrumentationClientPath))};\n`
    : "";

return `${userInstrumentationImport}import "vinext/instrumentation-client";

(i.e., drop the newline after the interpolation in the template literal). Totally optional.

[ask-bonk]

Review posted on PR #1671. Here's the summary:

Approved. The change is small, correct, and well-tested.

• Adds an explicit side-effect import of the user's instrumentation-client.{ts,js} at the top of the generated Pages Router client entry, before hydrateRoot() — matching Next.js's page-bootstrap.ts ordering.
• The JSON.stringify + normalizePathSeparators handles path escaping correctly, and the default parameter (= null) keeps backward compatibility.
• Both the presence and absence cases are covered by the new tests.
• One cosmetic nit noted (leading blank line when no instrumentation file exists) — non-blocking.

github run

[james-elicx]

Rebased on origin/main. Main added an options object as the 4th arg to generateClientEntry ({ appPrefetchRoutes }) while this PR added a positional instrumentationClientPath; merged both into the single options object ({ appPrefetchRoutes, instrumentationClientPath }) and updated the two test call sites in entry-templates.test.ts to match. vp check and the targeted entry-templates.test.ts (22 tests) pass.