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 1 commit intocloudflare:mainfrom
Conversation
commit: |
|
/bigbonk review this pr please |
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 |
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>
a29b027 to
814b37a
Compare
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