Repository navigation
docs(deploy): document deploy.runtime.memoryLimit - #3482
Conversation
avocet-bot
left a comment
There was a problem hiding this comment.
Review: #3482 — docs(deploy): document deploy.runtime.memoryLimit
Reviewed head SHA: 2f826f898dea09575435322dd79a4ac1ccd0d83e
Disposition: Comment (content correct; non-blocking merge-ordering note)
What this PR does
Deno Deploy lets you configure a project through a deno.json / deno.jsonc file. One of the keys under deploy.runtime sets the per-application runtime memory ceiling. This PR touches a single documentation file, deploy/reference/builds.md, and makes two changes:
- Renames the documented key
deploy.runtime.memory_limit(snake_case) todeploy.runtime.memoryLimit(camelCase) in the two places it appears — the "dynamic / non-static apps" option list and the "framework preset" option list. It also adds the clarifier ", in MB" and a sentence stating the oldmemory_limitspelling is "still accepted but deprecated." - Bumps the frontmatter
last_modified: 2026-09-30→2026-10-01.
The stated goal (PR body) is to align the documented spelling with the camelCase style used by the rest of Deno's config file (e.g. deploy.buildTimeout), on the strength of the engine change in denoland/deployng#3771.
This is a documentation-only change — there is no runtime code in the diff, so there are no code-level correctness, concurrency, or security concerns to evaluate. The review therefore focuses on whether the documentation is accurate, internally consistent, and whether it is safe to publish now.
Verified accurate (no action needed)
- "in MB" unit claim (
deploy/reference/builds.md:184,:195): correct. The engine treats this value as a megabyte count; the default 768 and the 4 GB Pro cap are consistent with MB, and the engine PR's own tests usememoryLimit: 2048/4096(2 GB / 4 GB). The added unit clarifier is a genuine improvement. - "
memory_limitstill accepted but deprecated": matches the intended engine semantics in denoland/deployng#3771, which keeps the snake_case key working as an alias. (One edge case the docs omit — #3771 rejects the config if both keys are present with different values — but omitting that corner is not misleading.) - Both occurrences updated: the dynamic-apps section (
:183) and the framework-preset section (:194) were both changed; none were missed. The JSONC code examples further down in the file do not include a memory-limit key, so no example now contradicts the prose. last_modifiedbump (:2): consistent with a real repo convention (sibling pages carry the field, e.g.deploy/reference/runtime.md→ 2026-06-18) and the new date equals today, 2026-10-01.- Not contradictions:
subhosting/manual/events.md:51"MEMORY_LIMIT"is an unrelated 502 error-code string;builds.md:122/:131"Runtime memory limit" are dashboard-UI prose labels, not thedeno.jsonkey. Both correctly left untouched. Markdown is well-formed (balanced backticks, preserved 4-space item / 6-space continuation indentation, no new or broken links).
Non-blocking finding — merge ordering: documents behavior that is not live yet
Background / intended behavior. The parser that consumes the deno.json deploy.runtime section lives in the deployng repository at packages/framework-detect/mod.ts. On the current main it reads only the snake_case key:
let runtimeMemoryLimit: number | undefined = undefined;
if (typeof runtime.memory_limit === "number") runtimeMemoryLimit = runtime.memory_limit;
else if (runtime.memory_limit !== undefined) return null;
There is no memoryLimit (camelCase) handling anywhere on main. Acceptance of the camelCase key, plus the "snake_case is now a deprecated alias" semantics this PR documents, are introduced by denoland/deployng#3771 ("feat: accept deno.json deploy.runtime.memoryLimit").
Why this matters. As of this review, deployng#3771 is OPEN / BLOCKED / unmerged (state: OPEN, mergedAt: null). This docs PR therefore presents memoryLimit as the primary, live spelling and demotes memory_limit to "deprecated" — the exact inverse of what ships in production today, where snake_case is the only accepted spelling and is not deprecated.
Practical impact if merged prematurely. A reader who follows the published docs and sets only memoryLimit in their deno.json would have it silently ignored by the live engine (the key is not read; no error is raised), so their application would quietly run at the 768 MB default instead of the configured limit. This is a concrete, user-misleading outcome.
Why this is non-blocking rather than a requested change. There is nothing to fix in the diff itself — the content is correct for the post-#3771 world. The author has already explicitly stated the gate in the PR body: "Should merge once denoland/deployng#3771 is deployed." This comment simply confirms that dependency is real and still outstanding. Recommendation: hold the merge until deployng#3771 is deployed (and, per that PR's own body, until the published @deno/framework-detect the CLI consumes is live).
Minor note (optional)
deploy/changelog.md:40-41 still documents deploy.runtime.memory_limit as the canonical field and links to this same builds reference. After this change the changelog presents the now-"deprecated" spelling as canonical. This is defensible as a point-in-time historical record, so it is not a blocker, but once #3771 ships the team may want to align or forward-link it for repo-wide consistency.
Review skill evidence
First substantive action was invoking the pr-review-toolkit:code-reviewer Claude Code subagent via the Task tool (run against head 2f826f8). It verified the two-hunk diff, read the full builds.md at head, grep'd the whole denoland/docs repo for memory_limit/memoryLimit occurrences, checked the last_modified frontmatter convention on sibling reference pages, and traced the actual deno.json parser in denoland/deployng (packages/framework-detect/mod.ts) plus the state of engine PR #3771. Its conclusions were independently corroborated here by re-checking deployng#3771's merge state and the sibling-page frontmatter.
Summary
The documentation content is correct, well-written, and internally consistent, and the unit/date edits are genuine improvements. The only risk is sequencing: the doc describes memoryLimit as live while the engine PR that enables it (deployng#3771) is still open. No changes requested; flagging the merge gate (already acknowledged by the author) and the optional changelog alignment.
avocet-bot
left a comment
There was a problem hiding this comment.
Review: #3482 — docs(deploy): document deploy.runtime.memoryLimit
Reviewed head SHA: aa00e6f4492ecf9b2c04b428b7f59fc84e37900f
Disposition: Approve
What this PR does
Deno Deploy lets you configure a project through a deno.json / deno.jsonc file. One of the keys under deploy.runtime sets the per-application runtime memory ceiling. This PR touches a single documentation file, deploy/reference/builds.md, and makes two changes:
- Renames the documented key
deploy.runtime.memory_limit(snake_case) todeploy.runtime.memoryLimit(camelCase) in the two places it appears — the "dynamic / non-static apps" option list and the "framework preset" option list. It also adds the clarifier ", in MB" and a sentence stating the oldmemory_limitspelling is "still accepted but deprecated." - Bumps the frontmatter
last_modified: 2026-09-30→2026-10-01.
The head commit (aa00e6f) is an empty "retrigger review after denoland/deployng#3771 deployed" commit; the file diff is identical to the earlier heads of this PR.
This is a documentation-only change — there is no runtime code in the diff, so there are no code-level correctness, concurrency, or security concerns to evaluate. The review focuses on whether the documentation is accurate, internally consistent, and safe to publish now.
Merge-ordering gate from the prior review round — now CLEARED
In earlier rounds the single finding was a non-blocking sequencing concern: the camelCase memoryLimit key is only honored once the engine change in denoland/deployng#3771 ("feat: accept deno.json deploy.runtime.memoryLimit") is merged and deployed. At that time #3771 was still open, so the docs described behavior that was not yet live.
That gate is now resolved, verified independently this round:
-
deployng#3771 is MERGED —
mergedAt: 2026-10-01T03:51:20Z, merge commit7cb99ab784ce88a058c930b28c738b10d82f0c3b. -
The camelCase key is live in the engine parser on deployng
main, inpackages/framework-detect/mod.ts(the module that parses thedeno.jsondeploy.runtimesection). Lines 508–520:// Parse memoryLimit - available for all modes including undefined (framework auto-config). // `memory_limit` is its deprecated snake_case spelling, accepted as an alias; giving both with // different values is invalid. if ( runtime.memoryLimit !== undefined && runtime.memory_limit !== undefined && runtime.memoryLimit !== runtime.memory_limit ) return null; const memoryLimit = runtime.memoryLimit !== undefined ? runtime.memoryLimit : runtime.memory_limit; let runtimeMemoryLimit: number | undefined = undefined; if (typeof memoryLimit === "number") runtimeMemoryLimit = memoryLimit; else if (memoryLimit !== undefined) return null;
runtime.memoryLimitis now the primary key;runtime.memory_limitis read only as a fallback alias and the engine's own comment labels it "deprecated snake_case." This matches the PR's wording ("The earlier spellingmemory_limitis still accepted but deprecated") precisely. The author's retrigger commit asserts the change is deployed, and the code is confirmed present onmain.
Verified accurate (no action needed)
- "in MB" unit claim (
deploy/reference/builds.md:184,:195): correct. The engine passes the number through raw (runtimeMemoryLimit = memoryLimit); the default 768 and the 4 GB Pro cap in the surrounding (unchanged) text are consistent with a megabyte count. - "
memory_limitstill accepted but deprecated": matches the live parser exactly, including the edge case that supplying both keys with different values invalidates the config (return null). - Both occurrences updated: the dynamic-apps section (
:183) and the framework-preset section (:194) were both changed; none were missed. The JSONC code examples further down in the file contain no memory-limit key, so no example contradicts the prose. last_modifiedbump (:2): consistent with a real repo convention (sibling pages carry the field, e.g.deploy/reference/runtime.md) and the new date equals today, 2026-10-01.- Not contradictions:
subhosting/manual/events.md:51"MEMORY_LIMIT"is an unrelated 502 error-code string;builds.md:122/:131"Runtime memory limit" are dashboard-UI prose labels, not thedeno.jsonkey — both correctly left untouched. Markdown is well-formed (balanced backticks, preserved list indentation, no new or broken links).
Minor note (optional, not blocking, out of scope for this PR)
deploy/changelog.md:41 still documents deploy.runtime.memory_limit as the field. Because snake_case remains an accepted (if deprecated) alias, this past-tense changelog entry is not a contradiction; the team may optionally align or forward-link it for consistency.
Review skill evidence
First substantive action was invoking the pr-review-toolkit:code-reviewer Claude Code subagent via the Task tool (run against head aa00e6f). Its specific charge this round was to independently verify whether the prior merge-ordering gate had cleared: it checked deployng#3771's merge state, fetched packages/framework-detect/mod.ts from deployng main, and quoted the parser lines proving the camelCase key is live with snake_case as a deprecated alias. Its conclusions were independently corroborated here by re-running gh pr view 3771 (MERGED) and grepping the same parser source.
Summary
The documentation content is correct, well-written, and internally consistent, and the unit/date edits are genuine improvements. The sole prior finding — docs describing not-yet-live behavior — is fully resolved now that deployng#3771 is merged (2026-10-01T03:51:20Z) and the camelCase memoryLimit handling is present in the engine parser on main. No blocking issues; approving.
Documents the camelCase
deploy.runtime.memoryLimitfromdenoland/deployng#3771, matching
deploy.buildTimeoutand the rest ofDeno's config file.
memory_limitis noted as a deprecated alias thatkeeps working.
Should merge once denoland/deployng#3771 is deployed.