fix(build): stop --dry-run deleting dist/ and honor build.outDir - #3566
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830a6aa4e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
830a6aa to
ce75d58
Compare
A dry run cleared the output directory before the dry-run guard, so `veryfront build --dry-run` printed "Dry run: no files will be written" and then deleted the whole of dist/ without writing anything back. `build.outDir` was parsed into the config and then dropped: the CLI hardcoded `dist`, so the documented way to point the build away from a project's own dist/ silently did nothing. Resolve the output directory from the loaded config in buildCommand (`-o/--output` > `build.outDir` > `dist`, relative paths against the project dir) and move the clean step behind the dry-run guard.
Honoring build.outDir made a previously inert config value load-bearing: setupBuildDirectories clears the output directory before writing, so `outDir: "."` or `".."` would have recursively deleted the project's own source or the workspace above it. resolveBuildOutputDir now rejects any output directory that is the project directory or contains it, for -o/--output as well as the config path (`veryfront build -o .` could already do this). It throws rather than falling back to dist, because silently substituting a different directory is the ignored-configuration bug this change exists to fix. Also add the missing defineConfig import to the deploying.md snippet so it compiles when pasted, matching every other config example under docs/guides/.
ce75d58 to
e3b8c68
Compare
Found on a DX dogfood walk: a fresh project was scaffolded,
veryfront buildwas run against it from outside the monorepo, and the output directory was
watched.
Symptom
Two separate ways for
veryfront buildto destroy a project'sdist/with nowarning.
1.
--dry-rundeletes the output directory. With three files indist/(
index.js,nested/deep.txt, a marker file):The directory and everything in it is gone, and nothing was written to replace
it. A developer who runs a dry run to preview a build loses their previous
build output.
2.
build.outDiris silently ignored. Withexport default { build: { outDir: "custom-out" } }inveryfront.config.ts,the build still cleared and wrote
dist/and reported4.63 KB in dist;custom-out/was never created. No warning, no error. The config file isloaded — the same file with
ssg: falsechanges the build's behaviour — sooutDirwas read into the config object and then dropped. That left-o/--outputas the only working escape hatch, and nothing in the CLI says so.
Root cause
setupBuildDirectories(src/build/production-build/build/build-setup.ts) ranan unconditional
adapter.fs.remove(outputDir, { recursive: true })beforeits
if (dryRun) returnguard, so a dry run cleaned the output directory andthen skipped everything that would have refilled it.
buildCommand(cli/commands/build/command.ts) hardcodedoptions.outputDir ?? join(options.projectDir, "dist"), computed beforegetConfig()was even called, soconfig.build.outDirnever reached the build.Fix
output directory, so stale artifacts from a previous build cannot leak into a
new one.
-o/--output>build.outDir>dist. A relativeoutDirresolves againstthe project directory.
displayBuildConfigmoved aftergetConfig()so thedisplayed, reported and written directory are the same one.
build.outDirmoves from "accepted for compatibility, no built-in semantics"to "core runtime/build" in
src/config/README.md, anddocs/guides/deploying.mdno longer tells readers the builder ignores it.
Regression tests
src/build/production-build/build/build-setup.test.ts— "leaves an existing output directory untouched in dry run" / "still clears the output directory for a real build"removeis a no-op, which is precisely why they could not see the deletion; these use an adapter that deletes for real.cli/commands/build/command.test.ts—resolveBuildOutputDircasesformatBuildOutputPathtests.tests/integration/server/build/build.test.ts— new "output directory" blockbuild.outDircase drivesbuildCommanditself, since that is where the directory is decided, and asserts on what the CLI prints (… in custom-out) — the exact line that read… in distbefore. Added inside the existing suite rather than a new file so no new sanitizer opt-out is introduced (lint:sanitizer-baselinestays at 404/404).Both integration tests were confirmed red before the fix: the dry-run one fails
with
NotFound … /dist/index.js, theoutDirone fails with the captured CLIoutput showing
4.63 KB in dist.Also verified against the real CLI on a scratch project outside the repo:
--dry-runnow writes and deletes nothing;build.outDir: "custom-out"writesto
custom-out/and leavesdist/intact;-o flagoutstill overrides theconfig; with no config the default is still
distand stale artifacts are stillcleared.
Review follow-up
Honoring
build.outDirmade a previously inert config value load-bearing, andreview caught the one new hazard that created: since the build clears its output
directory before writing, an
outDirof.or..would have recursivelydeleted the project's own source, or the workspace above it. A compatibility-era
config that was harmless while the field was ignored would have become
destructive.
resolveBuildOutputDirnow rejects any output directory that is the projectdirectory or contains it, and throws rather than quietly falling back to
dist—silently substituting a different directory is the ignored-configuration bug this
PR exists to fix. The guard covers
-o/--outputas well as the config path;veryfront build -o .could already delete the project before this PR, and aguard on only one of the two inputs to the same function is a half-guard. A
sibling directory such as
../shared-distis unaffected.Also added the missing
import { defineConfig } from "veryfront";to thedeploying.mdsnippet, matching every other config example underdocs/guides/.On a PR whose whole premise is following the published docs literally, a snippet
that does not compile when pasted is in scope.
Declined on the record:
--preset embeddedstill ignoresbuild.outDir. It alsoignores
--dry-run(it writes files anyway),--no-split,--no-compress,--prefetch,--ssg/--no-ssg,--includeand--exclude, and never callsgetConfig()at all —handleBuildCommandbranches tohandleEmbeddedBuildbefore the config is loaded and passes it exactly one option. That is a
pre-existing, uniform condition this PR neither created nor worsened, and fixing
outDiralone would leave the same silent-ignore for six other flags. Filed as#3585.
Not fixed here
A build that fails still leaves the developer with neither the old artifact
nor a new one, because the clean happens up front. Fixing that properly means
staging the build and swapping it in at the end —
createBuildPublicationinsrc/build/production-build/build/build-publication.tsalready does exactlythis for the asset pipeline and is the obvious vehicle — but routing the whole
production build through it is a much larger change than this one and belongs
in its own PR.