Allow users to run local dev or tests on static asset workers when the assets dir doesn't exist#13116
Conversation
🦋 Changeset detectedLatest commit: e7874c0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR relaxes the assets-directory-existence check for local dev / test paths while keeping it enforced for deploy. Good change overall, but I spotted a functional regression and some dead code left behind.
Issues (highest severity first)
-
Bug:
overridesparameter dropped inunstable_getMiniflareWorkerOptions— The old call wasgetAssetsOptions({ assets: undefined }, config, options?.overrides?.assets). The refactored call no longer passesoverrides, so any caller providing asset overrides will have them silently ignored. See inline suggestion. -
Dead code:
NonExistentAssetsDirErrorcatch is now unreachable ingetMiniflareOptionsFromConfig— SincevalidateDirectoryExistenceis nowfalse,getAssetsOptionswill never throwNonExistentAssetsDirError. The try/catch block (lines 273-290) and theNonExistentAssetsDirErrorimport are now dead code and should be cleaned up — just callgetAssetsOptions(...)directly without the try/catch. -
Minor: temp directory accumulation in Miniflare — Each
getServices()call when the directory doesn't exist creates a newmkdtempdirectory that is never cleaned up. During a long dev session with repeatedsetOptions()calls (e.g. file-watcher triggers while the build output still doesn't exist), empty temp directories will accumulate in/tmp. Not a big deal since the OS cleans/tmpeventually, but worth noting.
| processedAssetOptions = getAssetsOptions({ | ||
| args: { | ||
| assets: undefined, | ||
| }, | ||
| config, | ||
| // For getPlatformProxy/local dev we don't need to validate the directory's existence | ||
| validateDirectoryExistence: false, | ||
| }); |
There was a problem hiding this comment.
Dead code: since validateDirectoryExistence is now false, getAssetsOptions will never throw NonExistentAssetsDirError, making the surrounding try/catch block (through line 290) unreachable. Consider removing the try/catch and calling getAssetsOptions(...) directly.
This comment was marked as outdated.
This comment was marked as outdated.
dario-piotrowicz
force-pushed
the
dario/allow-dev-and-tests-when-no-assets-dir-exist
branch
from
35a7ee2 to
83fc91d
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
dario-piotrowicz
force-pushed
the
dario/allow-dev-and-tests-when-no-assets-dir-exist
branch
from
83fc91d to
5e3a05c
Compare
|
How does this compare to #13079? |
…e assets dir doesn't exist
dario-piotrowicz
force-pushed
the
dario/allow-dev-and-tests-when-no-assets-dir-exist
branch
from
5e3a05c to
e7874c0
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ${ | ||
| (formatConfigSnippet( | ||
| { compatibility_date: compatibilityDateStr }, | ||
| config.configPath | ||
| ), | ||
| false) | ||
| } |
There was a problem hiding this comment.
🔴 Comma operator causes error message to display "false" instead of config snippet
The expression (formatConfigSnippet({ compatibility_date: compatibilityDateStr }, config.configPath), false) uses JavaScript's comma operator: it evaluates the formatConfigSnippet() call, discards the returned string, and the whole expression evaluates to false. The error message shown when compatibility_date is missing during wrangler versions upload will contain the literal text false instead of the actual config snippet.
Comparison with correct usage in deploy.ts
The correct pattern is used in packages/wrangler/src/deploy/deploy.ts:557:
${formatConfigSnippet({ compatibility_date: compatibilityDateStr }, config.configPath, false)}Here false is passed as the third argument (the formatted parameter) to formatConfigSnippet, which controls JSON formatting. But in versions/upload.ts, false is instead the second operand of the comma operator.
| ${ | |
| (formatConfigSnippet( | |
| { compatibility_date: compatibilityDateStr }, | |
| config.configPath | |
| ), | |
| false) | |
| } | |
| ${formatConfigSnippet( | |
| { compatibility_date: compatibilityDateStr }, | |
| config.configPath, | |
| false | |
| )} |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@dario-piotrowicz do you have a link to an issue or something where this was discussed? I'm not 100% sure this is a good idea, and I think I would generally expect a missing assets dir to trigger a hard error. Vite deals with this by making the assets dir config field optional, which feels right to me. |
I thought there was a general consensus that we shouldn't hard error... 🤔 |
|
@dario-piotrowicz maybe I'm talking nonsense, and definitely feel free to disagree with me! But in general my thinking is:
|
Sounds good 👍 , I'll update the PR accordingly The only point that I don't fully agree is the last one, if users run something like |
3 participants
Users running local dev (
wrangler dev,vite dev) or tests (usingvitest-pool-workers) might want to specify an assets directory that not yet exists (because it gets built as part of the build process for example). The changes in this PR allow all the above mentioned to work even if the assets directory doesn't exist yet.wrangler deploy(and similar commands) still fails if the directory doesn't exist at deploy timeA picture of a cute animal (not mandatory, but encouraged)