Skip to content

feat: add optional dev-time next typegen#651

Open
pacexy wants to merge 13 commits intocloudflare:mainfrom
pacexy:feat/dev-next-typegen
Open

feat: add optional dev-time next typegen#651
pacexy wants to merge 13 commits intocloudflare:mainfrom
pacexy:feat/dev-next-typegen

Conversation

@pacexy
Copy link

@pacexy pacexy commented Mar 22, 2026

Summary

  • run automatically during dev server startup when Next.js is installed
  • rerun typegen when app/pages route files are added or removed, with a small debounce and serialized execution
  • add tests covering startup, opt-out via , route-file watch reruns, and missing-Next fallback behavior

Notes

  • this PR is intended as a temporary stopgap to improve developer experience
  • Vinext startup still succeeds when next is not installed; typegen is skipped in that case
  • the long-term goal is still native Vinext type generation rather than depending on next typegen

Testing

  • vp check
  • vp test tests/tsconfig-paths-vite8.test.ts tests/typegen.test.ts

@pacexy pacexy changed the title Add dev-time Next typegen support feat: add dev-time next typegen support Mar 22, 2026
Comment on lines +20 to +27
export function resolveNextTypegenBin(root: string): string | null {
try {
const projectRequire = createRequire(path.join(root, "package.json"));
return projectRequire.resolve("next/dist/bin/next");
} catch {
return null;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid have having a hard dependency/requirement on Next.js for Vinext to be usable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I agree Vinext itself should not depend on Next.js to be usable.

In this PR, my intent was only to add an optional dev-time integration: if next is installed in the project already, Vinext will opportunistically run next typegen; if it is not installed, startup still succeeds and we skip it.

For now, this is mainly intended as a pragmatic way to improve the developer experience in the short term. We can still follow up later with a native Vinext typegen implementation so this no longer depends on Next.js at all.

@james-elicx
Copy link
Collaborator

/bigbonk review

Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: add dev-time next typegen support

Thanks for the contribution — the implementation is clean and the test coverage is solid. However, I have a fundamental concern about the direction, plus a few implementation issues.

Fundamental concern: requiring Next.js at runtime

I agree with @james-elicx's comment. vinext's core value proposition is to replace Next.js with a Vite-based reimplementation. Requiring next to be installed for type generation undermines this. next typegen pulls in the entire Next.js framework at runtime — its next.config.* loader, internal route scanning, and TypeScript compiler integration.

The right long-term solution is for vinext to implement its own typegen that generates equivalent type definitions from the route tree it already scans internally (via routing/app-router.ts and routing/pages-router.ts). The route data is already available — the missing piece is writing the .d.ts files that Next.js's typegen produces.

If this is intended as a stopgap until vinext has its own typegen, that should be clearly documented (in the code, not just the README) and tracked with an issue.

Implementation issues

See inline comments for specific items.

});

typegen.start();
server.httpServer?.once("close", () => typegen.close());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cleanup path has two problems:

  1. Middleware mode: If Vite is running in middleware mode, server.httpServer is null and this listener never registers — the child process leaks.

  2. Not the idiomatic Vite pattern: The recommended way to clean up in configureServer is via the server.close hook (which Vite invokes regardless of httpServer presence).

Suggested change
server.httpServer?.once("close", () => typegen.close());
server.httpServer?.on("close", () => typegen.close());

Better yet, consider using the Vite-provided cleanup mechanism. You could move the typegen.close() call into the plugin's closeServer hook (if available in your Vite version) or store a reference and clean up there. At minimum, the ?. guard means this silently leaks in middleware mode.

return;
}

child = spawnImpl(process.execPath, [resolvedNextBin, "typegen"], {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing env: process.env is redundant — spawn inherits the parent environment by default when env is not specified. But more importantly, this means the child process inherits the full parent environment, which may include NODE_OPTIONS, NODE_ENV, etc. that could interfere with next typegen.

Consider either removing the explicit env or being selective about what's inherited.

Suggested change
child = spawnImpl(process.execPath, [resolvedNextBin, "typegen"], {
child = spawnImpl(process.execPath, [resolvedNextBin, "typegen"], {
cwd: root,
stdio: "ignore",
});


if (closed) return;

if (code !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next typegen exits with code 0 on success. However, it also exits 0 when it has nothing to do (e.g., no next.config.* found, no app/ directory). This means a misconfigured project will silently produce no types with no user feedback.

Consider adding a --verbose or debug-level log when typegen succeeds, so users can confirm it actually ran and generated files. Or at minimum, log on the first successful run:

if (code === 0 && !firstRunLogged) {
  firstRunLogged = true;
  logger.info("[vinext] `next typegen` completed.");
}

clearTimeout(timer);
timer = null;
}
if (child) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

child.kill() sends SIGTERM by default. If next typegen is in the middle of writing files, this could leave partial .d.ts files on disk. This is probably fine in practice since next typegen is fast, but worth noting.

Also, after child.kill(), setting child = null immediately means the exit handler (line 79) will still fire asynchronously and find child === null, then check closed === true and bail. This is correct but fragile — if someone later adds logic after the if (closed) return; check, it could run with stale state. Consider removing the exit listener before killing:

Suggested change
if (child) {
if (child) {
child.removeAllListeners();
child.kill();
child = null;
}

}

return {
start() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scheduleWithDelay(0) still defers via setTimeout. This means if configureServer calls typegen.start() and the server immediately starts handling requests, there's a window where types haven't been generated yet. This is fine for dev ergonomics (types arrive shortly after), but the name start() implies synchronous initiation. A comment would help:

Suggested change
start() {
start() {
// Deferred to avoid blocking configureServer. Types will be
// generated asynchronously after the event loop yields.
scheduleWithDelay(0);
},

README.md Outdated
});
```

For CI, `tsc --noEmit`, or production builds that rely on these helpers, run `next typegen` before your type-check step. vinext only auto-runs type generation in dev mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation tells users to run next typegen in CI, which means they need next as a dependency in their project. This is a significant ask for a framework that's supposed to replace Next.js. At minimum, note that this requires next as a devDependency and explain the tradeoff.

Also, the sentence "vinext only auto-runs type generation in dev mode" could be misleading — it only runs in dev mode and only when next is installed.

writeFile(root, "pages/index.tsx", "export default function Page() { return <div>hi</div>; }\n");
}

function installFakeNext(root: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fake next bin is a good testing strategy, but it means the tests don't validate that next typegen actually works (e.g., that it finds next.config.*, scans routes correctly, produces valid .d.ts files). This is acceptable for unit tests of the controller logic, but the test descriptions ("runs next typegen on dev server startup") slightly overstate what's being verified.

Consider adding a comment like:

/** Install a fake `next` binary that increments a counter file instead of running real typegen. */

}
});

it("logs once and skips when next is missing", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has a subtle race: start() and schedule() are called back-to-back. start() defers via setTimeout(fn, 0), and schedule() defers via setTimeout(fn, 150). Since the bin resolver returns null, start() will log when it fires, but schedule() will also attempt to log (hitting the missingNextLogged guard). The test asserts exactly 1 call, which is correct — but it's testing an implementation detail (the missingNextLogged flag) rather than the stated behavior.

The waitFor with info.mock.calls.length === 1 also has a timing assumption: it passes as soon as 1 call happens, but doesn't wait to confirm no second call arrives. A more robust approach:

await new Promise((resolve) => setTimeout(resolve, 300));
expect(info).toHaveBeenCalledTimes(1);

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 22, 2026

The review has been submitted. Here's a summary of the feedback I posted:

Request Changes on PR #651 with 8 inline comments covering:

  1. Fundamental architectural concern: vinext's goal is to replace Next.js, but this feature requires next to be installed at runtime. The route tree data is already available in vinext's routing modules — the better long-term approach is to generate the .d.ts files directly, without delegating to next typegen. This aligns with @james-elicx's earlier comment.

  2. Cleanup leak in middleware mode: server.httpServer?.once("close", ...) silently does nothing when httpServer is null (middleware mode), leaking child processes.

  3. Redundant env: process.env in spawn options — it's the default behavior and inherits potentially problematic env vars like NODE_OPTIONS.

  4. Silent success with no feedback: next typegen exits 0 even when it has nothing to do (no config file, no app dir), giving users no indication of whether types were actually generated.

  5. Fragile cleanup in close(): child.kill() without removing listeners first leaves the exit handler to fire asynchronously — correct now but fragile for future changes.

  6. Missing comment on deferred start(): scheduleWithDelay(0) isn't obvious — a comment explaining the async deferral would help.

  7. README documentation gap: Telling users to run next typegen in CI means they need next as a dependency, which contradicts vinext's positioning.

  8. Test race condition: The "logs once" test uses waitFor which passes on first match but doesn't verify no second call arrives — a fixed delay + assertion would be more robust.

github run

@pacexy pacexy changed the title feat: add dev-time next typegen support feat: add optional dev-time next typegen Mar 23, 2026
@james-elicx
Copy link
Collaborator

james-elicx commented Mar 23, 2026

I'm not completely convinced that delegating to Next.js in the CLI for this is the right move, whether short-term or not.

Considering that the codegen, from what I can tell, is primarily for creating types for dynamic segments, can we use the route information we already gather and do the codegen based on that ourselves?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants