Skip to content

perf(v4): skip the eager stack capture when building a ZodError - #6450

Merged
colinhacks merged 2 commits into
mainfrom
perf-cheap-zod-error
Aug 24, 2026
Merged

perf(v4): skip the eager stack capture when building a ZodError#6450
colinhacks merged 2 commits into
mainfrom
perf-cheap-zod-error

Conversation

@colinhacks

@colinhacks colinhacks commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Constructing a ZodError runs the Error constructor, and V8 captures a structured stack trace there. That capture is the dominant cost of a failed parse and roughly half the error's retained bytes, and it grows with the depth of the call stack. Zeroing Error.stackTraceLimit around the construction skips it.

The error stays a genuine Error. Checks with instanceof, Error.isError, util.types.isNativeError, Object.prototype.toString and structuredClone all behave exactly as they do today. What it gives up is the frames on an error returned by safeParse — the message header is still there.

The throwing path keeps its full stack, and gets faster too. Today it captures twice: once in the Error constructor and again in captureStackTrace, which trims to the caller. Skipping the first leaves one capture and the same frames.

main this branch
failing safeParse 250k 569k 2.28x
failing parse 145k 204k 1.40x

Across a wider set of failing safeParse shapes — z.string(), a 5-key object with one and with all keys bad, a 3-deep nested object, an array of 20, a discriminated union with no match — the range is 2.9x to 3.7x, against a control on valid input of 1.05x–1.09x. Retained bytes per returned error drop from 1073 B to 714 B, and from 1442 B to 708 B at call depth 40, so the cost no longer grows with stack depth. Gzipped bundle cost is +63 to +68 B, since every bundle carries core.ts.

Three alternatives do not work. Setting stackTraceLimit on the subclass is ignored, because V8 reads it only off Error — MDN is explicit that it is a static property with no per-subclass form. Assigning Error.prepareStackTrace suppresses the frames but leaves construction just as slow, so the eager cost is collecting them rather than formatting them. Dropping extends Error altogether measures the same as skipping only the constructor, so it would forfeit instanceof Error for nothing. Save, zero, construct, restore is what MDN documents and what Node core does internally (nodejs/node#24747).

Two engine cases fall back to a plain construction rather than the trick. Where Error.captureStackTrace is missing there is no way to put the frames back, so suppressing would strip the stack from a thrown error — Safari 11.1 through 17.1 has stackTraceLimit but not captureStackTrace. Where stackTraceLimit is absent or frozen, assigning would throw; SES lockdown() deletes it before hardening Error, and an app that hardens Error after import latches the trick off on the first failure. Each fallback fails a test when removed.

The cost worth naming explicitly is that a returned error carries no frames at all, so re-throwing one — if (!r.success) throw r.error — produces a thrown error with an empty trace where parse() would give five. Nothing downstream can recover them, since throw does not re-capture, and Node prints such an error as [ZodError: …].

Refs #6316, #5910

Copilot AI lite review requested due to automatic review settings August 20, 2026 14:33

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog 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.

ℹ️ No blocking issues. The mechanism holds up under everything I could throw at it — two notes inline, one about an edge case the PR body claims to cover and one about a new test that can't currently fail.

Reviewed changes — first pullfrog review on this PR (the only prior review was a Copilot quota notice), so this covers both commits rather than a delta.

  • Suppressed V8's stack capture during error constructionnewError() in core/core.ts zeroes Error.stackTraceLimit around new Definition() and restores it in a finally, taking effect only where $constructor was given a Parent.
  • Gated the trick on a writability probe_E resolves once at module load to Error or null, so non-V8 engines and realms that already hardened Error fall back to a plain new.
  • Pinned the error-construction contract — new classic test asserts instanceof Error, types.isNativeError, Object.prototype.toString, structuredClone, the frameless returned stack, and that the borrowed global is given back.
  • Added throwing-path coverage for mini and the AOT fallback — both assert the reinstalled stack is rooted at the parse call site.
  • Raised the three zod/mini bundle ceilings to 2906 / 3372 / 4390.

What I verified myself rather than taking on trust:

  • Only two Parent: sites exist in the tree (classic/errors.ts:78, core/errors.ts:296), both Parent: Error, so no schema construction goes through the zeroed window.
  • class Definition extends Error {} has no constructor body, so new Definition() reaches only the Error intrinsic — there is no reentrancy window in which user code could observe the zeroed limit. init() and its lazy message accessor run after the restore.
  • The stack property's descriptor is byte-for-byte identical between a limit-0 and a normal construction, before and after first read. Worth checking given how much this repo treats descriptors as contract.
  • The ceilings are honest: re-measuring the three fixtures here gives 2878 / 3344 / 4362, exactly the numbers in the comment, each with the 28 bytes of headroom the file calls for.
  • node --frozen-intrinsics takes the fallback correctly, and the full zod project is green (374 files, 4924 tests, no type errors).

ℹ️ Nitpicks

  • _E: any gives up the typing the previous revision had (ErrorConstructor & { stackTraceLimit: number }). Since _E is a module-level const, (ErrorConstructor & { stackTraceLimit: number }) | null narrows fine after the if (!_E) guard and costs nothing at runtime.
  • The rewritten ceiling comment drops the closing sentence that explained the per-fixture notes underneath it. Without it, the Also carries … comments on the next two fixtures read as unanchored, and the 2836 / 3299 / 4329 triple sitting directly above "zod-mini-boolean" invites being misread as belonging to that one fixture.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/zod/src/v4/core/tests/compile.test.ts Outdated
Comment thread packages/zod/src/v4/core/core.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated

@pullfrog pullfrog 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.

Important

Re-review at the same head as the previous pullfrog review — no new commits, so no new inline comments (its three threads still stand). Two things this run adds: ecosystem precedent that upgrades the open core.ts:22 thread from "narrow edge case" to "the one variant everyone else abandoned", and a trade-off nobody has raised yet.

Reviewed changes — the same two commits at 94c9b24, re-derived independently rather than as a delta.

  • Stack capture suppressed during error constructionnewError() at core/core.ts:26 zeroes Error.stackTraceLimit around new Definition() and restores it in a finally, reached only where $constructor was given a Parent.
  • Gated on a load-time writability probe_E resolves once to Error or null, falling back to a plain new.
  • Error-construction contract pinned — the new classic test covers instanceof Error, types.isNativeError, Object.prototype.toString, structuredClone, the frameless returned stack, and restoration of the borrowed global.
  • Throwing-path coverage added for zod/mini and the AOT compile fallback.
  • Three zod/mini ceilings raised to 2906 / 3372 / 4390, i.e. +70 / +73 / +62 B gzipped.

Confirmed independently this run: the PR body's claim that a thrown error is unaffected holds. Every $ZodRealError throw site routes through core/parse.ts:31 or :55 — including the AOT fast-path fallback in core/compile.ts, which delegates to the schema's own .parse with a callee override. $ZodAsyncError, $ZodEncodeError and $ZodCyclicError have no Parent: Error and never reach newError. Touched test files are green (82 tests, no type errors).

⚠️ Node adopted this exact trick, then had to move the writability check to each write

The open thread on core.ts:22 reads as a judgement call. The precedent makes it lopsided: nodejs/node#24747 — the citation in the PR body — is where Node adopted save-zero-restore in v11.5.0, and nodejs/node#38215 is where Node had to add a writability guard after --frozen-intrinsics broke core with the identical Cannot assign to read only property 'stackTraceLimit'. Vitest, this repo's own runner, calls isErrorStackTraceLimitWritable() before every write. Probing once at module load is the variant both moved away from, and ESM guarantees zod evaluates before an app-entry lockdown().

Technical details
# Load-time probe is the one form the precedent rejects

## Affected sites
- `packages/zod/src/v4/core/core.ts:22``_E` snapshots writability at module evaluation; nothing re-checks.
- `packages/zod/src/v4/core/core.ts:29`, `:33` — both assignments throw in strict mode if `Error` hardened after that snapshot. The throw escapes `newError`, so every failed parse throws instead of returning an error.

## Evidence gathered this run
- Reproduced on this branch (`pnpm dev`, node v24): import zod, `Object.freeze(Error)`, `z.string().safeParse(1)``TypeError: Cannot assign to read only property 'stackTraceLimit' of function 'function Error() { [native code] }'`.
- SES `lockdown()` with the default `errorTaming: "safe"` *deletes* `Error.stackTraceLimit` and then hardens `Error`, so the failure there is
  `TypeError: Cannot add property stackTraceLimit, object is not extensible` — the `!_E` fallback does not catch that shape either.
  `endojs/endo#8` documents this as a known pattern that breaks libraries.
- `nodejs/node#24747` (v11.5.0) introduced save-zero-restore in Node core; `nodejs/node#38215` later wrapped it in a writability check
  because `--frozen-intrinsics` surfaced the same `TypeError` from core libs.
- `vitest/dist/chunks/index.*.js:380``if (isErrorStackTraceLimitWritable()) Error.stackTraceLimit = 0;`, checked per write.

## Required outcome
A realm that hardens `Error` after zod's module graph evaluates must not turn every failed parse into a `TypeError`.
Alternatively, if that is judged out of scope, the PR body should stop claiming hardened realms are covered — the guard
only holds when the hardening precedes the import.

## Suggested approach (optional)
- Latch `_E` to `null` on the first failed assignment, so a post-`lockdown()` app degrades to today's behavior rather than crashing.
  The write is already inside a function that has a `try`; the cost is one `catch`.
- Re-checking the descriptor per call is what Node and vitest do, but that is a `getOwnPropertyDescriptor` on the failed-parse
  hot path — worth measuring against the latch before choosing, given the three axes.

## Open questions for the human
- Is SES `lockdown()` after import in scope at all? It is the only realistic route here outside a deliberate `Object.freeze(Error)`.

ℹ️ A re-thrown safeParse error now has zero frames, which is more than a console-rendering change

The PR body describes the visible effect as Node printing [ZodError: …]. Measured on this branch, the sharper statement is that if (!r.success) throw r.error — a very common pattern — produces a thrown error whose stack has 0 at frames, against 4 for the equivalent parse. throw does not re-capture, so nothing downstream can recover them, and error trackers fall back to grouping by message. That is a defensible price for 3–4x, but it is a product decision rather than a rendering detail, and the test it replaces (stack trace carries the message and the parse call site) was pinning exactly this.

Technical details
# Frames are unrecoverable on the re-throw path

## Affected sites
- `packages/zod/src/v4/core/parse.ts:81`, `:101``_safeParse` / `_safeParseAsync` construct the error and never call `util.captureStackTrace`, which is correct for the optimization but means the returned object's `stack` is final.
- `packages/zod/src/v4/classic/tests/error.test.ts:875` — the replaced test asserted the returned error's stack reached the call site.

## Measurement
```
re-thrown safeParse error frame count: 0
header: ZodError: [
thrown-by-parse frame count: 4
```

## Required outcome
Nothing in the code necessarily — but the PR body should describe the effect as "an error returned by `safeParse` has no stack frames,
including if you re-throw it" rather than as a Node inspection quirk, since that is what users will hit.

## Open questions for the human
- Is `throw result.error` common enough in the ecosystem to be worth a note in the release notes or the error-handling docs?

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog 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.

ℹ️ The latch fixes the crash — I reproduced the old failure and confirmed it's gone. One note inline: neither of the two new safety branches is actually covered by the test added for them.

Reviewed changes — delta from 94c9b24 to 3db9990. Both commits were rewritten by the force-push, so this is scoped by range-diff rather than SHA.

  • Replaced the load-time writability probe with a runtime latch_E now starts as Error and is set to null the first time Error.stackTraceLimit = 0 throws, so a realm that hardens Error after zod's import graph evaluates degrades instead of crashing.
  • Added a non-numeric bail-outtypeof saved === "number" skips the trick entirely when the property is absent, which is both the non-V8 case and the shape SES lockdown() leaves behind (it deletes the property before hardening).
  • Made the two throwing-path tests able to failcompile.test.ts and mini/error.test.ts now capture thrown outside the try instead of relying on expect.hasAssertions().
  • Dropped the hardcoded 10 from the restore assertion in favour of the ambient Error.stackTraceLimit.
  • Retuned the three zod/mini ceilings to 2900 / 3364 / 4390.

Confirmed by running it rather than reasoning about it:

  • The old crash is genuinely fixed. Post-import Object.freeze(Error) no longer throws out of safeParse — it latches, degrades to a normal framed error, stays degraded, and leaves stackTraceLimit untouched. The SES delete-then-harden shape takes the new non-numeric path with no throw.
  • The ceiling comment's arithmetic is exact. Reverting core.ts to main and rebuilding measures 2808 / 3271 / 4300 against 2872 / 3336 / 4362 on this branch — +64 / +65 / +62, and each ceiling is measured + 28. (The stale 4301 in the comment being replaced is why the ceiling delta reads as 61; the measured delta is 62.)
  • Removing the frameless-stack test didn't cost thrown-path coverage — error.test.ts:754, :768 and :792 still pin the reinstalled frames across parse, the codec entry points, and post-suspension async.
  • The latch being permanent is the right call, not a gap: under lockdown() the hardening is irreversible, so re-probing per call would only burn cycles.
  • Touched suites green — 567 tests across the three files in both the zod and compile-mode projects, no type errors, packages/treeshake green.

ℹ️ Nitpicks

  • mini/tests/error.test.ts:35 dereferences (thrown as Error).stack! without first asserting thrown is an Error. Its compile.test.ts sibling — changed in the same commit for the same reason — does assert it, so a regression that stops the parse throwing fails cleanly there and with Cannot read properties of undefined here.
  • mini/tests/error.test.ts:16-21 still keeps both its assertions inside an unguarded catch, which is the exact shape this commit just fixed three lines below it.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated
@colinhacks
colinhacks force-pushed the perf-cheap-zod-error branch from 5fb21ad to fa33550 Compare August 20, 2026 16:35

@pullfrog pullfrog 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.

ℹ️ The coverage gap is closed — I mutation-tested both bail-outs and each one now fails a test when removed. Only a comment-style nit left inline.

Reviewed changes — delta from 3db9990 to fa33550. Test-only; core/core.ts and packages/treeshake/bundle-size.test.ts are byte-identical to the previously reviewed revision.

  • Split the single bail-out test into one per branch — the old "an engine without a writable stackTraceLimit still builds a working error" installed { value: undefined, writable: true } and exercised neither branch it was named for.
  • Added a fresh-module test for the latchvi.resetModules() plus await import("zod/v4"), then Error.stackTraceLimit redefined non-writable, so the latch fires in a throwaway instance.
  • Added an accessor test for the non-numeric bail-out — a set that increments a counter makes "never written to" observable, which a data property could not.
  • Hoisted e2 out of the catch in zod/mini's "error inheritance", so its two assertions run unconditionally.
  • Added expect(thrown).toBeInstanceOf(Error) before the .stack! deref in zod/mini's thrown-stack test.

Confirmed by running it rather than reasoning about it:

  • Both bail-outs are genuinely covered now. Replacing typeof saved === "number" with true fails "an engine with no stackTraceLimit is never written to"; removing the try/catch around the write fails "a realm that hardens Error after import degrades instead of throwing" with TypeError: Cannot assign to read only property 'stackTraceLimit'. The PR body's "a test covers it" holds.
  • vi.resetModules() really does isolate. I appended a probe after the hardening test asserting the file's own statically-imported z still returns a frameless stack, ran the whole file, and it passed — so the latch stays in the throwaway instance rather than poisoning whatever runs after it. Both vitest configs set isolate: true, so it can't leak across files either.
  • Green at this head: 86 tests across the two error files under --project zod with no type errors, and 190 under compile-mode including core/tests/compile.test.ts. pnpm check:comments clean.

One thing I deliberately did not flag: _E = null itself is uncovered — removing only that line, keeping the catch, changes nothing observable, it just re-reads and re-throws on every later failed parse. That's cost, not behavior, and not worth a test.

ℹ️ Nitpicks

  • The two-element loop in the hardening test reads as if it pins the latch's permanence, but it can't distinguish latching from catching twice. Harmless as written.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated

@pullfrog pullfrog 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.

ℹ️ The new bail-out is load-bearing and correctly covered — I checked the engine claim against MDN's compat data and mutation-tested the guard. Three small inline corrections, one of them a version number that's off by a major.

Reviewed changes — delta from 82ac047 to 7b8f2fe. This is the first substantive change since 3db9990; the intervening revisions were test and comment prose.

  • Added a third bail-out gated on captureStackTrace_E at core.ts:22 now initializes to "captureStackTrace" in Error ? Error : null, so an engine that has stackTraceLimit but cannot reinstate frames opts out of the trick entirely rather than permanently losing the stack on a thrown error.
  • Expanded the core.ts:21 comment to cover both the new condition and the existing latch.
  • Added a test for the new bail-outvi.resetModules(), delete Error.captureStackTrace, fresh await import("zod/v4"), then assert a thrown error's stack still reaches the parse call site.

Confirmed by running it rather than reasoning about it:

  • The guard is not dead code, which was my main question given that typeof saved === "number" already covers engines without stackTraceLimit. MDN's browser-compat-data puts Safari's Error.stackTraceLimit at 11.1 and Error.captureStackTrace at 17.2, so there is a real ~5.5-year window of JSC versions where the existing bail-out passes and frames still can't be put back. The new condition is the only thing covering it.
  • It's genuinely covered. Reverting :22 to = Error fails the new test.
  • util.ts:396 already runs the identical probe and falls back to a no-op, so the two load-time checks agree. Deleting captureStackTrace after import doesn't break the restore either — util snapshots the function reference, not its presence.
  • Green at this head: 88 tests across both error files under --project zod with no type errors, 191 under compile-mode including core/tests/compile.test.ts. pnpm check:comments clean.

ℹ️ The PR body documents two bail-outs, but there are now three

The body walks through the hardened-realm latch and the missing-stackTraceLimit fallback, and this commit adds a third condition that isn't mentioned. It's the one with the widest real-world reach of the three — every Safari before 17.2 rather than a deliberately hardened realm — and it's also the one that turns "this is a V8 and JSC optimization" into "this is a V8 optimization". Worth a sentence, given how carefully the rest of the body records the mechanism.

Technical details
# PR body omits the `captureStackTrace` bail-out

## Affected sites
- PR description — the paragraph beginning "A realm can harden `Error` at any point" enumerates the latch and the non-numeric fallback, and stops there.

## Required outcome
- The body should name the third condition and why it exists: suppressing the capture is only recoverable where `parse()` can reinstate frames, so an engine with `stackTraceLimit` but no `captureStackTrace` opts out.
- The "Refs" line and the benchmark table both read as V8-only already; a reader comparing against the guard shape shouldn't have to infer the third branch from the source.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/error.test.ts
Comment thread packages/treeshake/bundle-size.test.ts Outdated
Constructing a ZodError runs the Error constructor, and V8 captures a
structured stack trace there. That capture is the dominant cost of a failed
parse and roughly half the error's retained bytes, and it grows with the
depth of the call stack.

Suppress it by zeroing Error.stackTraceLimit around the construction and
restoring it afterwards. The instance stays a genuine Error, so instanceof,
Error.isError, util.types.isNativeError and [object Error] all keep working;
what it gives up is the frames on an error returned by safeParse. parse()
puts real frames back, since it already calls util.captureStackTrace with the
caller on the way out.

The limit is a global static on Error with no per-subclass equivalent, and
temporarily zeroing it is the technique MDN documents and Node core uses
internally. Setting it on the subclass is ignored, and Error.prepareStackTrace
suppresses the frames without avoiding their collection, so neither is an
option.

A realm can harden Error at any point, including after this module is
evaluated, so the assignment is guarded and latches off permanently the first
time it fails. Engines with no stackTraceLimit at all take the same fallback.

@pullfrog pullfrog 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.

ℹ️ The re-measured ceilings are exact — I rebuilt both sides and got the recorded numbers to the byte. One leftover from the rebase inline.

Reviewed changes — delta from 7b8f2fe to 241ee64. The branch was rebased, so most of the range is base drift from #6448 and #6452; core/core.ts and all three test files are byte-identical to the previously reviewed revision.

  • Rebased onto d85472c4, which moves the zod-mini-object baseline since declared symbol keys land in every bundle containing z.object.
  • Re-measured and raised the three zod/mini ceilings to 2903 / 3366 / 4437, and rewrote the record above them.

Confirmed by measuring rather than trusting the comment: reverting core/core.ts to d85472c4 and rebuilding gives 2807 / 3270 / 4346 against 2875 / 3338 / 4409 on this branch — +68 / +68 / +63, exactly what the comment now claims, with 28 bytes of headroom on each. That also explains why the previous run's +67 / +68 / +65 read as off by one: main's own boolean and string measurements drifted down a byte with the rebase. packages/treeshake is green, as are 88 tests across both error files under --project zod with no type errors and 147 under compile-mode.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/treeshake/bundle-size.test.ts

@pullfrog pullfrog 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.

ℹ️ Both threads from the last review are genuinely closed — I re-ran the mutation and the failure is clean now. Two nits inline on the same test, neither of them from this commit's lines.

Reviewed changes — delta from 241ee64 to 171dcb6. git diff-tree -r --stat is a single file, +5 / -3; core/core.ts, bundle-size.test.ts, core/tests/compile.test.ts and mini/tests/error.test.ts are byte-identical to the previously reviewed revision, so the ceiling measurements verified there still stand.

  • Corrected the Safari window in the captureStackTrace bail-out comment — "before 18.2" → "11.1 through 17.1", which matches MDN's compat data (stackTraceLimit 11.1, captureStackTrace 17.2) and bounds the range on both ends rather than just the top.
  • Moved the thrown-error assertions below the finallythrown is hoisted, and expect(thrown).toBeInstanceOf(Error) now guards the .stack deref the way its two siblings already did.

Confirmed by running it rather than reasoning about it:

  • The assertion move does what it was supposed to. Reverting the core.ts:22 gate to = Error now fails with expected 'ZodError: […' to contain 'callSite' at :925, against the TypeError: Error.captureStackTrace is not a function it produced before. The test can still fail, and it fails legibly.
  • vi.resetModules() doesn't need undoing here. It only invalidates the cache for later dynamic import(), so it can't reach back into the file's static z binding; the throwaway instance's module-level _E is a separate variable from the one the rest of the file uses.
  • Green at this head: 88 tests across both error files under --project zod with no type errors, 147 under compile-mode, pnpm check:comments clean. pnpm lint:check exits 0 — the suppression warning below is a warning, not a CI failure.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated
Comment thread packages/zod/src/v4/classic/tests/error.test.ts Outdated
…stack

Turns the two commented-out `instanceof Error` assertions into real ones, and
adds a test that a returned error is still a native Error with a frameless
stack, and that the borrowed Error.stackTraceLimit is given back unchanged.
Adds a test for the fallback branch taken where the limit is not assignable,
which is the whole safety story off V8.

Adds compiled-fallback parity for z.compile, and raises the three zod/mini
bundle ceilings, measured.
@colinhacks
colinhacks force-pushed the perf-cheap-zod-error branch from 9b1c93d to 1015e4b Compare August 24, 2026 22:04

@pullfrog pullfrog 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.

✅ No new issues found. Both nits from the last review are closed, and the descriptor rework doesn't cost the test its ability to fail.

Reviewed changes — delta from 9b1c93d to 1015e4b. git diff-tree -r --stat is a single file, +2 / -3; core/core.ts, bundle-size.test.ts, core/tests/compile.test.ts and mini/tests/error.test.ts are byte-identical to the previously reviewed revision, so the ceiling measurements verified at 241ee64 still stand.

  • Restored Error.captureStackTrace through a captured descriptorObject.getOwnPropertyDescriptor at :904 and Object.defineProperty in the finally at :920, replacing the bare re-assignment.
  • Dropped the dead biome-ignore suppression above the deleteperformance/noDelete no longer exists in Biome 2.

Confirmed by running it rather than reasoning about it:

  • The descriptor round-trip does what it was supposed to. Measured both shapes directly: the old bare assignment recreated the property with enumerable: true and left Object.keys(Error) as ['stackTraceLimit', 'captureStackTrace']; the new defineProperty gives back enumerable: false and ['stackTraceLimit']. The test now matches its two siblings below it, which already restored stackTraceLimit this way.
  • The rework didn't cost the test its teeth. Reverting the core.ts:22 gate to = Error still fails at :924 with expected 'ZodError: […' to contain 'callSite' — the failure comes from the assertion, not from a TypeError thrown out of the restore.
  • The suppression is genuinely gone, not just moved. biome lint on this file is now silent; it was the only warning here.
  • The zod-mini-object note reads correctly against its ceiling now. Re-checked the arithmetic in place: measured + 28 gives 2903 / 3366 / 4437 on the nose, and the trimmed #6448 line no longer carries a superseded measurement.
  • Green at this head: 88 tests across both error files under --project zod with no type errors, pnpm check:comments clean.

Every inline thread on this PR is now resolved.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@colinhacks
colinhacks merged commit 5825605 into main Aug 24, 2026
15 checks passed
@colinhacks
colinhacks deleted the perf-cheap-zod-error branch August 24, 2026 22:31
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