feat: handle edge cases in plainObject, improve performance#5972
feat: handle edge cases in plainObject, improve performance#5972belgattitude wants to merge 3 commits into
Conversation
|
TL;DR — Replaces the Key changes
Summary ｜ 2 files ｜ 3 commits ｜ base: Tighter plain-object detection
The new check is both stricter about what counts as plain and simpler — four lines of prototype comparisons replace the five-step constructor dance.
Edge cases locked in by tests
Notably,
|
|
Some bench from the zod benchmark suite: cpu: AMD Ryzen 7 8845HS w/ Radeon 780M Graphics Current implementationNew Implementation |
There was a problem hiding this comment.
Important
The new isPlainObject is cleaner and faster, but it silently changes observable parse behavior for z.record() (and intersection merging) on inputs like Object.create({ ... }), which used to be accepted and now produce invalid_type. That's worth surfacing in the PR description, and the cross-realm branch is loose enough to be worth tightening.
A couple of specific things:
-
Object.create(someNonNull)now failsz.record(). The old impl treated any object whose prototype chain reachedObject.prototypeas plain (because it ultimately checked for an ownisPrototypeOfonctor.prototype). Under the new impl, only direct{}/Object.create(null)/ cross-realm objects are plain, soz.record(...).parse(Object.create({ foo: 1 }))now fails where it previously succeeded. The new test atindex.test.ts:846locks this in without calling it out. I'd mention it explicitly in the PR body — it's the kind of thing users hit via inherited-config patterns. -
The cross-realm branch is too permissive.
Object.getPrototypeOf(proto) === nullisn't specific tovm.runInNewContext({}); it also matchesObject.create(Object.create(null)),Object.setPrototypeOf({}, Object.create(null)), and any other two-deep chain ending in null. If the goal is genuinely cross-realm{}and nothing else,Object.prototype.toString.call(o) === "[object Object]"is a sharper test, or compareproto.constructor?.name === "Object". -
Proxy coverage is weaker than the tests imply.
new Proxy(new Date(), { getPrototypeOf: () => Object.prototype })passes the new check as a plain object, because thegetPrototypeOftrap is authoritative. The existing Proxy assertions only work because no trap is installed and the chain falls through to the target. Worth either adding a trap-based counterexample to document the limit, or dropping the Proxy-guarantee framing from the PR description.
The perf improvement and the Object.create(null) / vm-context handling are genuine wins. Thanks for the benchmarks.
Claude Opus ｜ 𝕏
| proto === null || | ||
| proto === Object.prototype || | ||
| // Required to support node:vm.runInNewContext({}) | ||
| Object.getPrototypeOf(proto) === null |
There was a problem hiding this comment.
Object.getPrototypeOf(proto) === null also returns true for Object.create(Object.create(null)) and Object.setPrototypeOf(new Date(), Object.create(null)) — any prototype chain that's exactly two deep with a null top. If the intent is strictly cross-realm {}, consider Object.prototype.toString.call(o) === "[object Object]" or proto.constructor?.name === "Object" for a tighter check.
| test("isPlainObject", () => { | ||
| expect(z.core.util.isPlainObject({})).toEqual(true); | ||
| expect(z.core.util.isPlainObject(Object.create(null))).toEqual(true); | ||
| expect(z.core.util.isPlainObject(Object.create({}))).toEqual(false); |
There was a problem hiding this comment.
Worth calling out in the PR description that this is a user-visible behavior change: z.record(...).parse(Object.create({ foo: 1 })) now rejects with invalid_type where it previously succeeded (the old impl accepted anything whose chain reached Object.prototype). Not necessarily wrong — it matches other libs — but it's a breaking parse-behavior change worth surfacing.
| get(target, _prop, _receiver) { | ||
| return target; | ||
| }, | ||
| }))).toEqual(false) |
There was a problem hiding this comment.
These Proxy assertions only hold because no getPrototypeOf trap is installed. new Proxy(new Date(), { getPrototypeOf: () => Object.prototype }) passes as plain under the new impl. Consider adding that as a counterexample, or phrasing the PR goal as "transparent proxies behave like their targets" rather than "Proxy is properly detected".
There was a problem hiding this comment.
Important
The new implementation matches @httpx/plain-object exactly, which is what the PR claims. Nice perf win. Two asks before merging: (1) call out the user-visible behavior change for Object.create(plainObj) in the PR description, since it flips a parsing result for z.record(...) and z.intersection(...) on inputs that previously round-tripped, and (2) add a tripwire test for the vm.runInNewContext case that motivates the 2-level prototype walk.
isPlainObject is part of the public surface via z.core.util.isPlainObject and is consumed internally by shallowClone, mergeValues (intersections) at core/schemas.ts:2508, record parsing at core/schemas.ts:2881, and normalizeObjectLikeDef at core/util.ts:653/681, so any behavioral flip ripples into those code paths.
Claude Opus ｜ 𝕏
| // Required to support node:vm.runInNewContext({}) | ||
| Object.getPrototypeOf(proto) === null | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is a user-visible behavior change worth documenting in the PR description: Object.create(plainObj) flips from true to false. The diff's test additions at index.test.ts:847-848 encode the flip but don't frame it as breaking.
Concrete impact:
const defaults = { host: "localhost" };
const cfg = Object.create(defaults);
cfg.port = 3000;
z.record(z.string(), z.any()).parse(cfg); // was: ok — becomes: invalid_typeSame story for z.intersection(...) — mergeValues at core/schemas.ts:2508 gates on isPlainObject(a) && isPlainObject(b), so transformed outputs that previously merged will now throw Unmergable intersection. The direction of the flip matches lodash / is-plain-obj / @sindresorhus/is consensus (the old zod behavior was the outlier), so the change is defensible — just worth a line in the PR body so Colin can decide whether to call it out in release notes.
| proto === null || | ||
| proto === Object.prototype || | ||
| // Required to support node:vm.runInNewContext({}) | ||
| Object.getPrototypeOf(proto) === null |
There was a problem hiding this comment.
The comment names node:vm.runInNewContext({}) as the reason for the 2-level walk, but no test exercises that path — the only case that lands on this branch in the added tests is the synthetic Object.create(Object.create(null)). Worth a tripwire so a future simplification to a single-level check doesn't silently regress cross-realm support:
import vm from "node:vm";
expect(z.core.util.isPlainObject(vm.runInNewContext("({ a: 1 })"))).toEqual(true);
expect(z.core.util.isPlainObject(vm.runInNewContext("[]"))).toEqual(false);
expect(z.core.util.isPlainObject(vm.runInNewContext("new Date()"))).toEqual(false);
Hello here's a proposal to fix some edge cases and improve isPlainObject performance
Tests for cases that weren't (imho) properly checked by the current implementation, notably:
Object.create({})shouldn't be a plain object (open to discuss, see risk below)Proxy, they should behave like their targetsRisk:
Other info
The implementation is taken from https://github.com/belgattitude/httpx/tree/main/packages/plain-object#readme (I'm the author). Note that the tests are passing CI node 20, 22, 24, 26, bun, cloudflare, edge, playwright chromium and deno.
I've tested the performance against the zod implementation to see if there won't be regression. On the specific benchmarks, node 24 -> 1.22x faster, bun 1.3.13 -> 2.76 faster.
Note that benchs might not be realistic from the zod perspective.
Open to discuss
Node 24.15.0
Bun 1.3.13