feat: add optional dev-time next typegen#651
feat: add optional dev-time next typegen#651pacexy wants to merge 13 commits intocloudflare:mainfrom
next typegen#651Conversation
next typegen support
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
We should avoid have having a hard dependency/requirement on Next.js for Vinext to be usable
There was a problem hiding this comment.
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.
|
/bigbonk review |
There was a problem hiding this comment.
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.
packages/vinext/src/index.ts
Outdated
| }); | ||
|
|
||
| typegen.start(); | ||
| server.httpServer?.once("close", () => typegen.close()); |
There was a problem hiding this comment.
This cleanup path has two problems:
-
Middleware mode: If Vite is running in middleware mode,
server.httpServerisnulland this listener never registers — the child process leaks. -
Not the idiomatic Vite pattern: The recommended way to clean up in
configureServeris via theserver.closehook (which Vite invokes regardless of httpServer presence).
| 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"], { |
There was a problem hiding this comment.
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.
| child = spawnImpl(process.execPath, [resolvedNextBin, "typegen"], { | |
| child = spawnImpl(process.execPath, [resolvedNextBin, "typegen"], { | |
| cwd: root, | |
| stdio: "ignore", | |
| }); |
|
|
||
| if (closed) return; | ||
|
|
||
| if (code !== 0) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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:
| if (child) { | |
| if (child) { | |
| child.removeAllListeners(); | |
| child.kill(); | |
| child = null; | |
| } |
| } | ||
|
|
||
| return { | ||
| start() { |
There was a problem hiding this comment.
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:
| 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. |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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);|
The review has been submitted. Here's a summary of the feedback I posted: Request Changes on PR #651 with 8 inline comments covering:
|
next typegen supportnext typegen
|
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? |
Summary
Notes
nextis not installed; typegen is skipped in that casenext typegenTesting