fix: add directory to wrangler assets config for static export#221
fix: add directory to wrangler assets config for static export#221arifpro wants to merge 6 commits intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review this pr please |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the fix — the core idea (detect output: 'export' and add directory to the wrangler assets config) is correct and addresses the issue. A few things need attention before this can merge.
Summary of issues
-
Hardcoded
"export"directory may not match the actual build output. The directory name is hardcoded, but the static export system uses a caller-providedoutDir— if that ever changes (or a user overrides it), the wrangler config would point to the wrong directory. At minimum this should have a comment explaining the assumption, or better, derive the value from where the build actually writes to. -
mainand worker entry are still generated for static export. Whenoutput: 'export', there's no Worker script — it's a static site. The generatedwrangler.jsoncstill includes"main": "./worker/index.ts"andgetFilesToGeneratestill createsworker/index.ts. These are unnecessary (and potentially confusing) for a static-only deployment. This is a larger concern but worth flagging. -
No tests for the new behavior. The
generateWranglerConfigtest suite should cover the static export case: verifydirectory: "export"is present in assets, verify theimagesbinding is omitted, and verify the non-export case is unchanged. The existing tests all usedetectProjectwhich returnsoutput: "", so the new code paths are untested. -
getOutputModeis async butdetectProjectis sync. The PR mutatesinfo.outputafter construction (info.output = await getOutputMode(root)), which is a bit awkward. It works, but it meansdetectProjectreturns incomplete data that must be patched up by the caller. Consider either makingdetectProjectasync or documenting this limitation clearly.
|
Review submitted. Here's a summary of the feedback I posted on PR #221: Requested changes with 4 inline comments:
Additionally noted the lack of tests — the new |
arifpro
force-pushed
the
fix/219-wrangler-assets-directory
branch
from
8b390b2 to
a29b027
Compare
When `output: "export"` is set in next.config, wrangler requires the `assets.directory` property. This reads the output mode via loadNextConfig (avoiding unnecessary resolveNextConfig overhead) and adjusts the generated wrangler.jsonc: - Adds `directory` to assets config via shared STATIC_EXPORT_DIR constant - Omits `main` (no Worker script needed for static sites) - Omits `images` binding (images pre-optimized at build time) - Omits `assets.binding` (no Worker to expose assets to) - Skips worker/index.ts generation in getFilesToGenerate Closes cloudflare#219 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
arifpro
force-pushed
the
fix/219-wrangler-assets-directory
branch
from
a29b027 to
814b37a
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for the fix. The core approach — detecting output: 'export' and conditionally adjusting the wrangler config — is sound and addresses the reported issue. The code is well-commented and the tests cover the new generateWranglerConfig and getFilesToGenerate behavior.
However, there are a few issues that need to be addressed before this can merge. The previous review from @ask-bonk raised some valid points; I'll focus on what I think is most actionable.
Key issues
-
The
STATIC_EXPORT_DIRconstant is declared but not actually consumed by the static export build. The comment says it's "used by bothgenerateWranglerConfigand the static export build step so they stay in sync" — but the static export build (staticExportPages/staticExportApp) takesoutDiras an external parameter and doesn't reference this constant. The constant only prevents drift withindeploy.ts, which is good, but the comment overstates its scope. More importantly, you need to verify that the actual build output lands in a directory calledexportrelative to the wrangler config, or this will silently break at deploy time. -
The
deploy()function still callsrunBuild()unconditionally for static export. The standard Vite/Cloudflare build (createBuilder().buildApp()) is designed for Worker deployments, not static export. Does this actually produce the right output for a static-only site? The build would need to produce anexport/directory with pre-rendered HTML. Without integration-level validation that the fullvinext deploypipeline works end-to-end foroutput: 'export', this PR only fixes the wrangler config shape but may fail at a different point. -
loadNextConfigis heavier than needed forgetOutputMode.loadNextConfigdoes a dynamicimport()of the config file, which evaluates the full module (including any plugin wrappers like@next/mdxthat may have side effects). TheresolveNextConfigstep (async redirects/rewrites/headers) is correctly skipped, which is good. But the import itself can still fail in edge cases (missing deps, TS compilation issues). The fallback to""on error is appropriate for robustness.
Minor
- The tests correctly validate the wrangler config output and worker file generation. Good coverage for the config layer.
| * Directory name for static export output. Used by both `generateWranglerConfig` | ||
| * (assets.directory) and the static export build step so they stay in sync. | ||
| */ | ||
| const STATIC_EXPORT_DIR = "export"; |
There was a problem hiding this comment.
The comment claims this constant is "used by both generateWranglerConfig and the static export build step so they stay in sync." However, staticExportPages and staticExportApp in build/static-export.ts accept outDir as an external parameter — they don't import or reference STATIC_EXPORT_DIR.
This means there's no actual compile-time guarantee that the build output directory matches what the wrangler config expects. The constant only prevents drift within this file, which is fine, but the comment should be corrected to avoid giving a false sense of safety.
More critically: who is responsible for passing outDir = "export" to the static export build? The deploy() function calls runBuild() which runs createBuilder().buildApp() — that's the standard Cloudflare Workers build, not the static export path. Is the static export build even wired into the deploy flow?
| const STATIC_EXPORT_DIR = "export"; | |
| /** | |
| * Directory name for static export output. | |
| * Used by `generateWranglerConfig` (assets.directory). | |
| * | |
| * NOTE: The static export build (`staticExportPages`/`staticExportApp`) accepts | |
| * `outDir` as an external parameter — if you change this value, the caller that | |
| * invokes the static export build must also be updated to match. | |
| */ | |
| const STATIC_EXPORT_DIR = "export"; |
| assets: { | ||
| not_found_handling: "none", | ||
| // For static export, wrangler serves files directly from the build output directory. | ||
| ...(isStaticExport && { directory: STATIC_EXPORT_DIR }), |
There was a problem hiding this comment.
The directory value here is relative to the wrangler config file location. For this to work, the Vite build must produce static files in a directory called export/ at the project root (next to wrangler.jsonc).
Have you verified this end-to-end? The runBuild() function (line 1032) calls createBuilder().buildApp() for App Router, which writes to dist/. I don't see where a static export would produce an export/ directory. If the build output doesn't match, wrangler will fail with a "directory not found" error at deploy time — replacing one deploy error with another.
Could you add an E2E test or at least document how output: 'export' is expected to work with the full deploy pipeline?
| // Detect output mode from next.config (async — reads config file). | ||
| // Must run before any code that reads info.output (e.g. getFilesToGenerate, | ||
| // generateWranglerConfig). detectProject is sync so this is set separately. | ||
| info.output = await getOutputMode(root); |
There was a problem hiding this comment.
The post-construction mutation pattern works but is fragile, as the previous review noted. The comment you added ("Must run before any code that reads info.output") is a good safeguard.
One concern: getFilesToGenerate is called at line 1170, which reads info.output — that's fine since it runs after this line. But getMissingDeps at line 1147 doesn't check info.output today. If someone later adds logic to skip certain deps for static export (e.g., no need for @vitejs/plugin-rsc), they'd need to know about this ordering requirement.
Consider adding output to the return value of detectProject by making it accept a pre-loaded config, or restructuring so the async step happens inside deploy() before detectProject returns. This isn't a blocker but would make the code less surprising.
|
Review submitted successfully. Here's a summary of what I posted: Requested changes with 3 inline comments:
The top-level summary also noted that |
- Fix STATIC_EXPORT_DIR comment to not overstate its scope (static export build callers still pass outDir independently) - Clarify getOutputMode doc comment to explain why loadNextConfig is used instead of the heavier resolveNextConfig - Export runBuild for testability - Skip the Workers build for output:'export' — call createBuilder only for Worker deployments; static exports use pre-built files in export/ and throw clearly if the directory is missing - Add runBuild tests covering the static export fast-path and the missing-directory error case
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for the contribution. The core idea — detecting output: 'export' and conditionally adjusting the wrangler config — is the right approach, and the test coverage for config generation is solid. However, there are a few issues that need to be resolved before this can merge.
Blocking issues
1. No actual integration with the static export build pipeline
This is the most critical issue. runBuild() for the non-export path calls createBuilder().buildApp() (App Router) or build() (Pages Router), which produces Worker output in dist/. For static export, the PR skips the build entirely and expects an export/ directory to already exist — but nothing in the deploy pipeline produces that directory.
The static export functions (staticExportPages/staticExportApp in build/static-export.ts) accept outDir as an external parameter, but they're never called from runBuild() or deploy(). This means vinext deploy with output: 'export' will always fail with "Static export directory not found" unless the user manually runs a separate build step that somehow produces an export/ directory. The error message suggests next build or vinext build, but neither of those will produce a directory called export at the project root.
This PR replaces one deploy error (wrangler's "missing directory property") with another ("Static export directory not found"). The runBuild path for static export should actually invoke the static export build, or at minimum this limitation should be clearly documented in the issue/PR.
2. getMissingDeps still installs Worker-only deps for static export
When output === 'export', the deploy flow still installs @cloudflare/vite-plugin, @vitejs/plugin-rsc, and react-server-dom-webpack. A pure static deployment doesn't need any of these — it's just files served by Cloudflare's asset hosting. This is unnecessary overhead and confusing for users.
3. Vite config generation is wrong for static export
getFilesToGenerate still generates a vite.config.ts with cloudflare() plugin and vinext() for static export projects. Since runBuild skips the Vite build entirely for static export, this file is dead weight. Worse, the viteConfigHasCloudflarePlugin check at line 1294 will throw if no vite config exists and the cloudflare plugin wasn't detected — but for static export, this check is irrelevant.
Minor issues
4. not_found_handling: "none" may be wrong for static export
For a Worker-backed site, "none" makes sense because the Worker handles all routing. For a purely static site, users likely want "single-page-application" (serves index.html for missing paths, enabling client-side routing) or "404-page" (serves a custom 404 page). "none" means any path without a matching static file returns a bare 404, which breaks client-side routed SPAs. This should at least be configurable, or default to something more useful for static export.
5. STATIC_EXPORT_DIR comment is misleading
The comment says the constant is used by runBuild for the static export outDir, but runBuild only checks for the directory's existence — it doesn't pass this value to any build function. The static export functions in build/static-export.ts accept outDir externally and don't reference this constant. There's no compile-time guarantee these stay in sync.
| } | ||
| console.log(`\n Static export: using pre-built files in ${STATIC_EXPORT_DIR}/\n`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This early return means runBuild never actually produces the static export — it only checks if the directory already exists. But nothing in the deploy() pipeline creates it.
staticExportPages and staticExportApp in build/static-export.ts are the functions that generate the static files, but they're never called here. The error message tells users to run next build or vinext build, but neither of those produces an export/ directory at the project root by default (Next.js uses out/, and vinext's static export uses whatever outDir the caller passes).
This should either:
- Wire up the actual static export build (call
staticExportApp/staticExportPageswithoutDir = STATIC_EXPORT_DIR), or - Be clearly documented as a known limitation requiring a manual pre-build step, with the correct command to run.
| // Static exports are pure asset deployments — no Worker script needed. | ||
| ...(isStaticExport ? {} : { main: "./worker/index.ts" }), | ||
| assets: { | ||
| not_found_handling: "none", |
There was a problem hiding this comment.
For a static-only deployment (no Worker), not_found_handling: "none" means any request for a path without a matching static file gets a bare 404 from Cloudflare's asset serving. This breaks client-side routing (React Router, etc.) where the SPA needs index.html served for all paths.
Consider defaulting to "single-page-application" for static export, or at least "404-page" if the user has a custom 404.html. "none" is appropriate when a Worker handles routing, but there's no Worker here.
|
|
||
| if (!info.hasWorkerEntry) { | ||
| // Static exports are pure asset deployments — no Worker script needed. | ||
| if (!info.hasWorkerEntry && info.output !== "export") { |
There was a problem hiding this comment.
Worker entry is correctly skipped, but vite.config.ts is still generated at line 1098 with cloudflare() and vinext() plugins. Since runBuild skips the Vite build for static export, this file serves no purpose.
Similarly, the viteConfigHasCloudflarePlugin check at line 1294 will run even for static export and could throw a confusing error if the user has a vite config without the cloudflare plugin — which is perfectly valid for a static-only deployment.
Both should be gated on info.output !== "export".
| * `outDir` as an external parameter — if you change this value, the call sites | ||
| * in `runBuild` must also be updated to match. | ||
| */ | ||
| const STATIC_EXPORT_DIR = "export"; |
There was a problem hiding this comment.
The comment says this is used by runBuild for the static export outDir, but runBuild only uses it to check if the directory exists — it doesn't pass this value to any build function. The actual static export build functions (staticExportPages/staticExportApp) accept outDir as a parameter and don't reference this constant.
| const STATIC_EXPORT_DIR = "export"; | |
| /** | |
| * Directory name for static export output. | |
| * Used by `generateWranglerConfig` (assets.directory) and `runBuild` | |
| * (existence check before deploy). | |
| * | |
| * NOTE: The static export build (`staticExportPages`/`staticExportApp`) accepts | |
| * `outDir` as an external parameter — if you change this value, the call sites | |
| * that invoke the static export build must also be updated to match. | |
| */ | |
| const STATIC_EXPORT_DIR = "export"; |
| // Detect output mode from next.config (async — reads config file). | ||
| // Must run before any code that reads info.output (e.g. getFilesToGenerate, | ||
| // generateWranglerConfig). detectProject is sync so this is set separately. | ||
| info.output = await getOutputMode(root); |
There was a problem hiding this comment.
The post-construction mutation pattern is fragile. detectProject returns output: "" and the caller patches it afterward. Any code between detectProject() and this line sees the wrong value.
This works today because getFilesToGenerate and generateWranglerConfig are called later (line 1285), but it's easy to break during future refactors. Consider making detectProject accept an optional pre-loaded output mode, or restructuring the deploy function so the async config load happens first and the result is passed into detectProject.
|
Review submitted successfully. Here's a summary of what I posted: Requested changes with 5 inline comments covering 3 blocking issues and 2 minor issues: Blocking:
Minor:
|
Replace stale runBuild tests (old logic checked for pre-built export/ dir) with tests that mock dynamic imports and verify the function completes. Add new tests covering detectProject output param, getMissingDeps skipping Worker-build deps for static export, getFilesToGenerate skipping vite.config.ts for static export, and generateWranglerConfig emitting not_found_handling: 404-page.
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for this PR — the core direction is right, and I appreciate the effort to address the previous review feedback (making detectProject accept output upfront, wiring up the actual static export build, skipping Worker-only deps/files). The config generation layer and its tests are solid. However, there are a few issues that need attention before this can merge.
Summary
Blocking
-
runBuildtests don't actually test anything meaningful —createServeris a top-level import that isn't mocked, so the tests will call the real VitecreateServeragainst a bare temp directory (nopackage.json, no React, no actual pages). The tests likely pass becausecreateServerfails and the error path isn't validated, or because the mock setup doesn't work as intended withvi.doMockfor top-level imports. These tests give false confidence. -
resolveNextConfigcalled with wrong signature — At line 1168,resolveNextConfigis called asresolveNextConfig({ output: "export", root: info.root }), but the actual function signature isresolveNextConfig(config: NextConfig | null, root?: string).rootis a separate second parameter, not a property of the config object. This meansrootwill default toprocess.cwd()and the synthetic config will have an unexpectedrootproperty. -
not_found_handling: "404-page"is subtly wrong for many static export apps — Static export from Next.js often produces SPAs with client-side routing."404-page"serves404.htmlfor missing paths, which breaks client-side routing (React Router,next/linkwith shallow routing)."single-page-application"is more appropriate as the default since it servesindex.htmlfor all unmatched paths. At minimum this should be configurable rather than hardcoded.
Non-blocking
-
Duplicate code in the App Router and Pages Router branches of
runBuild(the warning/error logging + success message). Minor, but worth extracting. -
The
compatibility_flags: ["nodejs_compat"]flag is still included for static export deployments. A pure static asset deployment doesn't neednodejs_compatsince there's no Worker running.
| const port = typeof addr === "object" && addr ? addr.port : 0; | ||
| const baseUrl = `http://localhost:${port}`; | ||
| const outDir = path.join(info.root, STATIC_EXPORT_DIR); | ||
| const config = await resolveNextConfig({ output: "export", root: info.root }); |
There was a problem hiding this comment.
Bug: resolveNextConfig signature is resolveNextConfig(config: NextConfig | null, root?: string) — root is the second positional parameter, not a property on the config object.
As written, root will default to process.cwd() (not info.root), and the resolved config will have an unexpected root property that Next.js config doesn't define.
| const config = await resolveNextConfig({ output: "export", root: info.root }); | |
| const config = await resolveNextConfig({ output: "export" }, info.root); |
| not_found_handling: "none", | ||
| // For static export SPAs, use "404-page" so the CDN serves the custom | ||
| // 404.html generated at build time rather than a blank 404 response. | ||
| not_found_handling: isStaticExport ? "404-page" : "none", |
There was a problem hiding this comment.
For apps using client-side routing (which is very common with output: 'export' — it's the standard SPA pattern), "404-page" will break navigation. Any direct URL access to a client-side route (e.g. /dashboard/settings) that doesn't have a matching pre-rendered HTML file will get 404.html instead of index.html.
"single-page-application" is the correct default for static export SPAs — it serves index.html for all unmatched paths, allowing the client-side router to handle them. For fully pre-rendered static sites (no client routing), "404-page" would be correct.
Since vinext can't easily distinguish between the two at config generation time, "single-page-application" is the safer default. Users who want "404-page" can override it in their wrangler.jsonc.
| not_found_handling: isStaticExport ? "404-page" : "none", | |
| not_found_handling: isStaticExport ? "single-page-application" : "none", |
| })); | ||
|
|
||
| try { | ||
| await expect(runBuild(info)).resolves.toBeUndefined(); |
There was a problem hiding this comment.
These tests don't validate what they claim to. createServer is imported at the top of deploy.ts (line 22) — it's bound at module load time, so vi.doMock on the dynamically-imported modules doesn't affect it. When runBuild calls createServer(...) against a bare temp directory, one of two things happens:
- It throws (no package.json, no React, etc.) and the test passes because
resolves.toBeUndefined()doesn't catch the rejection properly, or - The test never actually exercises the
staticExportApp/staticExportPagesmock paths.
To test this properly, you'd need to either:
- Mock
createServerat the module level withvi.mock('vite', ...)before the import - Extract the server creation into an injectable dependency
- Or restructure the test to provide a real minimal Vite project
As-is, these tests provide false confidence. Consider either making them work correctly or removing them in favor of the existing integration-level static export tests in tests/static-export.test.ts.
| const server = await createServer({ | ||
| root: info.root, | ||
| configFile: false, | ||
| plugins: [vinextPlugin({ appDir: info.root })], |
There was a problem hiding this comment.
Minor: appDir is also a standard Vite config option name (for App Router directory detection). Passing info.root as appDir to the vinext plugin could be confusing — it's setting the app root, not the app/ directory. Does the plugin actually need this option when root is already set? If so, a comment would help clarify.
| compatibility_date: today, | ||
| compatibility_flags: ["nodejs_compat"], | ||
| main: "./worker/index.ts", | ||
| // Static exports are pure asset deployments — no Worker script needed. |
There was a problem hiding this comment.
A pure static asset deployment doesn't need nodejs_compat since there's no Worker script running. Including it is harmless but misleading — it suggests a Worker is present.
Consider gating this on !isStaticExport:
| // Static exports are pure asset deployments — no Worker script needed. | |
| ...(isStaticExport ? {} : { compatibility_flags: ["nodejs_compat"] }), |
|
Review submitted successfully. Here's a summary of the feedback posted on PR #221: Requested changes with 5 inline comments covering 3 blocking issues and 2 non-blocking items: Blocking
Non-blocking
|
- Fix resolveNextConfig call: root is 2nd positional param, not object property
- generateWranglerConfig: use 'single-page-application' (not '404-page') for
static export not_found_handling so client-side routing works on direct URL
access; omit compatibility_flags for pure asset deployments (no Worker)
- Update STATIC_EXPORT_DIR comment: remove overclaiming about build sync;
clarify static-export.ts takes outDir as an external parameter
- Add clarifying comment on vinextPlugin({ appDir }) in runBuild
- Update tests: reflect single-page-application and absence of
compatibility_flags for static export; rename runBuild test descriptions
to be honest about what they exercise (createServer is top-level-bound)
|
/bigbonk review |
…c export These fields are harmless for pure asset deployments and keeping them makes the generated wrangler.jsonc consistent between SSR and static export, reducing the diff and making it easier to switch output modes later.
There was a problem hiding this comment.
Thanks for this PR — it addresses a real issue and the latest revision shows good progress. The config generation layer (generateWranglerConfig, getFilesToGenerate, getMissingDeps) is well-structured with solid test coverage. The getOutputMode function correctly uses loadNextConfig instead of the heavier resolveNextConfig, and the detectProject refactor to accept output upfront is cleaner than post-construction mutation.
However, there are a few issues that need to be addressed before this can merge.
Blocking
1. resolveNextConfig is called with a synthetic config, discarding the user's real config
At line 1176, resolveNextConfig({ output: "export" }, info.root) creates a minimal synthetic config. This means the user's actual next.config.ts settings — basePath, trailingSlash, pageExtensions, images, i18n, etc. — are all discarded. The static export build will use defaults for everything.
This should load the user's real config:
const rawConfig = await loadNextConfig(info.root);
const config = await resolveNextConfig(rawConfig, info.root);loadNextConfig is already imported at the top of the file (line 32) and used in getOutputMode. The vinext plugin on the Vite dev server will also load the config independently, so the two copies should stay in sync.
2. runBuild tests don't test what they claim
vi.doMock with paths like "../packages/vinext/src/index.js" targets the test-relative module specifier, but createServer is imported at the top of deploy.ts as a static import from "vite" and is bound at module load time — it's the real Vite createServer. This means the tests spin up a real Vite dev server against a bare temp directory (no package.json, no React, no real pages).
Whether the vi.doMock calls actually intercept the await import("./index.js") etc. inside deploy.ts depends on Vitest's module interception, but even if they do, the real createServer call will either fail silently or succeed vacuously. These tests provide false confidence about the static export orchestration.
Consider either:
- Mocking
createServerfrom"vite"at the module level withvi.mock("vite", ...) - Or removing these tests in favor of E2E coverage (the comment already references
tests/static-export.test.ts)
Non-blocking
3. Duplicate result-logging code
The warning/error/success logging block (lines 1189-1197 and 1223-1231) is duplicated verbatim between the App Router and Pages Router branches. Consider extracting it into a helper to reduce duplication.
4. not_found_handling: "single-page-application" is the right default but the wrong choice for fully pre-rendered sites
The current choice of "single-page-application" is a good safe default for SPAs with client-side routing. However, fully static-exported Next.js sites (where every page is pre-rendered) would benefit from "404-page" instead. This isn't blocking since users can override it in their wrangler.jsonc, but it's worth considering whether generateWranglerConfig should accept a hint (e.g., from generateStaticParams coverage) to choose the right strategy.
5. Minor: configFile: false means the vinext plugin won't see user's vite.config.ts
At line 1159, configFile: false prevents Vite from loading the user's vite.config.ts. This is probably intentional (to avoid the cloudflare() plugin being loaded for a static build), but it also means any user-defined Vite plugins, aliases, or config won't apply to the static export build. Worth a comment explaining the rationale.
| const baseUrl = `http://localhost:${port}`; | ||
| const outDir = path.join(info.root, STATIC_EXPORT_DIR); | ||
| // resolveNextConfig(config, root) — root is the second positional param | ||
| const config = await resolveNextConfig({ output: "export" }, info.root); |
There was a problem hiding this comment.
Bug: This creates a synthetic config with only output: "export", discarding the user's real next.config.ts settings (basePath, trailingSlash, pageExtensions, images, i18n, etc.). The static export build will use defaults for everything.
loadNextConfig is already imported at the top of the file and used in getOutputMode. Use it here to load the real config:
| const config = await resolveNextConfig({ output: "export" }, info.root); | |
| const rawConfig = await loadNextConfig(info.root); | |
| const config = await resolveNextConfig(rawConfig, info.root); |
| })); | ||
|
|
||
| try { | ||
| await expect(runBuild(info)).resolves.toBeUndefined(); |
There was a problem hiding this comment.
Issue: createServer is a static top-level import in deploy.ts (line 22) — it's bound at module load time, so vi.doMock doesn't replace it. This test spins up a real Vite dev server via createServer against a bare temp directory (no package.json, no React, no actual pages).
The test likely passes because either:
createServerthrows andresolves.toBeUndefined()doesn't catch it correctly, or- Vite tolerates the empty project but the mocked dynamic imports mean no actual static export work happens — so the test validates that "nothing crashes when nothing happens"
To properly test the static export orchestration, you'd need to mock createServer itself (via vi.mock("vite", ...) at the module level) or use a real minimal project fixture.
Consider removing these tests and relying on the integration coverage in tests/static-export.test.ts (which the comment already references), or restructuring them to actually validate the control flow.
| for (const warning of result.warnings) { | ||
| console.warn(` Warning: ${warning}`); | ||
| } | ||
| for (const error of result.errors) { | ||
| console.error(` Error [${error.route}]: ${error.error}`); | ||
| } | ||
| console.log( | ||
| `\n Static export complete: ${result.pageCount} pages → ${STATIC_EXPORT_DIR}/\n`, | ||
| ); |
There was a problem hiding this comment.
Nit: This warning/error/success logging block is duplicated verbatim in the Pages Router branch (lines 1223-1231). Consider extracting it:
function logStaticExportResult(result: StaticExportResult) {
for (const warning of result.warnings) {
console.warn(` Warning: ${warning}`);
}
for (const error of result.errors) {
console.error(` Error [${error.route}]: ${error.error}`);
}
console.log(
`\n Static export complete: ${result.pageCount} pages → ${STATIC_EXPORT_DIR}/\n`,
);
}|
|
||
| const server = await createServer({ | ||
| root: info.root, | ||
| configFile: false, |
There was a problem hiding this comment.
Nit: configFile: false means user-defined Vite plugins, aliases, and other config from vite.config.ts won't apply to the static export build. This is probably intentional (to avoid loading the cloudflare() plugin), but it's worth a brief comment explaining why, since it's a subtle behavioral difference from the normal Worker build path.
|
Review submitted successfully. Here's a summary of what I posted: Requested changes with 4 inline comments covering 2 blocking issues and 3 non-blocking items: Blocking
Non-blocking
|
|
I think I'll focus on #274 first and then re-visit this after that's working and prerendering outputs. |
|
Hi there, I appreciate the time you put into this. We've added a full pre-rendering pipeline separately, which includes static export pre-rendering, and so I've raised a PR to just add in the output directory to the config by itself (#569) and raised an issue for potential options around how we deal with the worker in static exports (#566) it would be good to get your input there if you're interested :) |
2 participants
When using output: 'export' in next.config.ts, wrangler 4.69.0 requires
the assets config to include a directory property. This fix:
pre-optimized at build time)
Fixes #219