feat: add instrumentation-client support

[hyoban]

No description provided.

[pkg-pr-new]

Open in StackBlitz

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

commit: 215900e

[southpolesteve]

/bigbonk review

[ask-bonk]

Review: instrumentation-client support

Well-structured PR — the architecture is clean: separate state module to avoid circular imports, virtual module aliasing to swap in the user's file or an empty fallback, and the dev-only timing plugin. Test coverage is solid across App Router, Pages Router, and src/ directory layouts, with E2E tests ported from Next.js.

A few things to address:

Issues

1. empty-module.ts produces {}, not null — The instrumentation-client.ts bridge casts instrumentationClientHooks as ClientInstrumentationHooks | null, but import * as x from "empty-module" yields {}, never null. Functionally harmless (optional chaining handles it), but the type cast is misleading. Consider checking for an empty namespace instead.

2. onRouterTransitionStart missing for Pages Router Link navigations — In link.tsx, the hook only fires inside the App Router branch (__VINEXT_RSC_NAVIGATE__). Pages Router Link clicks go through the Router.push/Router.replace path without calling onRouterTransitionStart. This may be intentional (Next.js's onRouterTransitionStart is likely App Router-only), but if so, worth a comment.

3. Duplicate optimizeDeps.entries computation for Pages Router + Cloudflare — The else if (hasCloudflarePlugin) branch sets environments.client.optimizeDeps.entries, then the if (!hasAppDir) block below also sets viteConfig.optimizeDeps.entries with identical values. The top-level optimizeDeps has no effect when environment-level config is present for that environment, so this is a no-op duplication. Consider extracting the entries computation or guarding the fallback block.

4. Fixture vite.config.ts imports changed from "vinext" to source paths — Both app-basic and pages-basic now import from "../../../packages/vinext/src/index.js" instead of "vinext". This diverges from how the other existing fixtures work and adds the need for tsc in the Playwright server commands. Was this intentional, or a debugging artifact? If needed for the new files to be picked up, it should be documented.

Nits

5. The onRouterTransitionStart helper function is duplicated identically in link.tsx:126-131 and navigation.ts:516-521. Consider extracting it into instrumentation-client-state.ts as a convenience wrapper.

6. The app-with-src Playwright project doesn't have a testMatch property, so it uses the default testDir matching. This is fine since testDir is set to ./tests/e2e/app-with-src, but noting it differs from the pattern used by other projects.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback:

Key issues flagged:

1. empty-module.ts produces {}, not null — The namespace import from the empty fallback yields an empty object, not null. The type cast in instrumentation-client.ts is misleading. Suggested using Object.keys().length > 0 to properly distinguish.

2. onRouterTransitionStart not called for Pages Router Link navigations — The hook only fires in the App Router branch of link.tsx. Pages Router Link clicks don't trigger it. Needs clarification on whether this is intentional.

3. Duplicate optimizeDeps.entries computation — For Pages Router + Cloudflare, the same entries array is computed twice (environment-level and top-level), with the top-level one being a no-op.

4. Fixture vite.config.ts changed from "vinext" to source paths — Both app-basic and pages-basic now import from source (../../../packages/vinext/src/index.js), which required adding tsc to Playwright commands. Questioned whether this is intentional or a debugging leftover.

Nits: Duplicated onRouterTransitionStart helper in link.tsx and navigation.ts; dev-only timing plugin confirmed as intentional.

github run

[hyoban]

resolved

[james-elicx]

/bigbonk review

[ask-bonk]

Review: instrumentation-client support

Clean architecture — the state module split avoids circular imports, normalizeClientInstrumentationHooks properly filters for the actual export rather than relying on type casts, and the virtual module aliasing via private-next-instrumentation-client is a solid pattern. The notifyAppRouterTransitionStart helper centralizes the hook invocation well, and the call sites in link.tsx, navigation.ts, and app-browser-entry.ts cover push, replace, and traverse without double-firing.

Test coverage is strong: unit tests for file discovery, integration tests for optimizeDeps.entries, E2E tests for timing guarantees, navigation hooks, HMR reload, and src/ directory layout. All ported from Next.js with proper attribution.

One issue to address and a few nits below.

[ask-bonk]

Review submitted. Here's a summary of the feedback:

Overall: Well-structured PR. The architecture is clean — separate state module to avoid circular imports, normalizeClientInstrumentationHooks properly checks for actual function exports (addressing the earlier empty-module concern from the first review), and the virtual module aliasing via private-next-instrumentation-client is solid. No double-notification issues across the three call sites (link.tsx, navigation.ts, app-browser-entry.ts).

Comments posted:

1. optimizeDeps.entries duplication (index.ts:2454) — For Pages Router + Cloudflare, the unconditional top-level optimizeDeps.entries block creates dead config since the environment-level config was already set. Suggested guarding with !hasCloudflarePlugin.

2. Dev-only timing plugin (instrumentation-client.ts:32) — The 16ms threshold is good but worth a comment explaining why there's no prod equivalent.

3. Inconsistent hook invocation pattern (app-browser-entry.ts:268) — The popstate handler directly calls getClientInstrumentationHooks()?.onRouterTransitionStart?.(...) instead of using the notifyAppRouterTransitionStart wrapper that the other two call sites use. Suggested using the wrapper for consistency.

4. vp run vinext#build added to Playwright commands (playwright.config.ts:13) — Asked whether this is feature-specific or a general fix, since it adds build time to every E2E run.

5. 20ms busy-wait in fixture (instrumentation-client.ts:9) — Noted this runs on every page load in the app-basic fixture, not just instrumentation-client tests.

github run

[hyoban]

resolved

[james-elicx]

/bigbonk review

[ask-bonk]

Review: instrumentation-client support

Clean PR overall. The architecture is well-structured — the state module split avoids circular imports, normalizeClientInstrumentationHooks correctly filters for actual function exports (addressing the earlier empty-module → {} concern), and the virtual module aliasing via private-next-instrumentation-client is solid. Test coverage is strong across App Router, Pages Router, and src/ directory layouts, with proper Next.js test suite attribution.

Two issues and two nits below.

[ask-bonk]

Review submitted. Here's a summary of the feedback I posted:

Issues:

1. normalizeClientInstrumentationHooks is too narrowly scoped (instrumentation-client-state.ts:8) — It only checks for onRouterTransitionStart. If Next.js adds more hooks in the future, the user's module would be silently discarded. Suggested checking for any function-valued exports instead, since call sites already use optional chaining.

2. notifyAppRouterTransitionStart fires for hash-only changes (navigation.ts:541) — The notification fires before the isHashOnlyChange early return check on line 548, meaning in-page anchor scrolls trigger the hook. Suggested moving the call after the hash-only guard or adding a comment if this matches Next.js behavior.

Nits:

3. insertPos = 0 edge case (plugins/instrumentation-client.ts:20) — The fallback when no imports exist is correct but a brief comment would help readers understand the intent.

4. Busy-wait scope in fixture (app-basic/instrumentation-client.ts:8) — Confirmed the if guard correctly scopes the 20ms delay to /instrumentation-client* paths only, so this doesn't affect other tests.

github run

[hyoban]

resolved