Skip to content

docs(deploy): document deploy.buildTimeout in seconds or with a unit suffix - #3483

Merged
piscisaureus merged 1 commit into
mainfrom
deno-json-build-timeout-seconds
Oct 1, 2026
Merged

piscisaureus merged 1 commit into
mainfrom
deno-json-build-timeout-seconds

Conversation

@piscisaureus

Copy link
Copy Markdown
Member

Documents the new deploy.buildTimeout format from
denoland/deployng#3772: a whole number of seconds, or a whole number
with an s, m or h suffix, raised to at least one minute and capped
to the plan maximum. The Next.js example uses "15m".

Should merge once denoland/deployng#3772 is deployed.

@avocet-bot avocet-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #3483 — document deploy.buildTimeout in seconds or with a unit suffix

Reviewed head SHA: 063acf4eeff9dcbed9cf1d60b5a7fc6602cb5b9e
Verdict: ✅ Approve — no blocking issues. The documentation accurately reflects the upstream implementation; examples are valid and internally consistent.

Review skill evidence

Invoked the pr-review-toolkit:code-reviewer subagent (via the Task tool) against the full diff for a correctness/consistency pass. It returned no findings at or above threshold and one sub-threshold wording observation (noted below). I then independently cross-checked every documented rule against the upstream implementation and tests in denoland/deployng#3772 to confirm the prose matches real parser behavior.

What this PR changes (background for a reader new to this area)

Deno Deploy lets an application declare its build configuration in deno.json under a deploy section (build command, framework preset, runtime, etc.). One of those keys, deploy.buildTimeout, bounds how long the build step may run before the platform aborts it. This PR edits a single documentation file, deploy/reference/builds.md, to describe a new accepted format for that key and to update the Next.js example accordingly. No code is changed in this repository.

The format change itself lives upstream in denoland/deployng#3772 ("take the deno.json build timeout in seconds or with a unit suffix"). Previously buildTimeout was a whole number interpreted as minutes. The upstream change reinterprets a bare number as a whole number of seconds and additionally accepts a string with an s/m/h unit suffix. This is a breaking reinterpretation of bare numbers, which is why the docs and the example both needed to change.

Correctness verification (docs vs. actual parser)

I confirmed each documented claim against the upstream parser parseDurationSeconds and the timeout-clamping function getEffectiveBuildTimeout, plus their tests:

  • "a whole number of seconds (600)" — matches typeof value === "number" → Number.isSafeInteger(value) && value >= 1. 600 is accepted (= 10 minutes). ✓
  • "a whole number with an s, m or h suffix ("90s", "10m", "1h")" — matches the regex /^([1-9][0-9]*)([smh])$/. All three examples are valid. ✓
  • Invalid examples "(such as 0, "10" or "1.5m")" cause the whole deploy section to be ignored — each maps to a real rejection path and each appears (directly or equivalently) in the upstream returns null for invalid buildTimeout test loop:
    • 0 → fails the value >= 1 number guard → null.
    • "10" → a string with no unit suffix → no regex match → null.
    • "1.5m" → the integer group [1-9][0-9]* rejects the fractional 1.5 → null.
      Returning null from buildConfigFromDenoJson is what "causes the whole deploy section to be ignored," so the prose is accurate. ✓
  • "A value above your plan's maximum (5 min Free / 30 min Pro) is capped to that maximum, and a value below 1 minute is raised to 1 minute" — matches getEffectiveBuildTimeout's Math.max(MIN_BUILD_TIMEOUT /* 1 */, Math.min(configuredTimeout, maxTimeout)). The new upstream test confirms 0.5→1, 60→30 (Pro), and 20→5 (Free). ✓
  • "Defaults to 5 minutes" — unchanged from the prior docs and consistent with DEFAULT_BUILD_TIMEOUT. ✓

The Next.js example change (buildTimeout: 15 → "15m")

This change is correct and necessary. Under the old semantics a bare 15 meant 15 minutes. Under the new semantics a bare 15 means 15 seconds, which would then be raised to the 1-minute floor — a silent behavior change if left as-is. Switching to the string "15m" preserves the original 15-minute intent, and 15 minutes is within the 30-minute Pro-plan maximum. ✓

Markdown / rendering

List structure, 2-space continuation-line indentation, and inline code spans all match the surrounding bullets and are balanced. No rendering breakage.

Non-blocking observations (not reasons to withhold approval)

  1. Dropped word "positive". The old text read "positive whole number of minutes"; the new text reads "a whole number of seconds." Taken in isolation, 0 is a whole number, which creates a slight tension with the separate rule "a value below 1 minute is raised to 1 minute." In practice the doc resolves this by explicitly listing 0 among the ignored values, so a reader is not misled (positive sub-minute values like 30 are raised; 0 is rejected). If an easy tweak is desired, restoring "positive" (e.g. "a positive whole number of seconds") would remove the ambiguity entirely. Optional.
  2. Merge ordering. Upstream denoland/deployng#3772 is still open/unmerged at the time of review, and the parser behavior documented here is not yet deployed. The PR description already acknowledges this ("Should merge once denoland/deployng#3772 is deployed."), so this is a release-sequencing note for the author rather than a docs-correctness defect. Worth confirming the upstream change actually ships with the behavior as written before/at merge, since the docs become live immediately on merge.

Summary

The documentation is accurate against the real upstream parser and clamping logic, all six examples (3 valid, 3 invalid) are internally consistent with the stated rules, the Next.js example migration is correct, and the markdown is clean. Approving. The only caveat is the process note that the upstream feature should be deployed before (or together with) merging these docs.

@piscisaureus
piscisaureus merged commit 06b37d2 into main Oct 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants