fix(form-core): treat non-plain objects with no own enumerable keys as unequal in evaluate#2140
fix(form-core): treat non-plain objects with no own enumerable keys as unequal in evaluate#2140Zelys-DFKH wants to merge 1 commit intoTanStack:mainfrom
Conversation
…s unequal in evaluate
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis patch fixes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
🎯 Changes
Fixes #1628.
evaluate()compares objects by iteratingObject.keys(). That works fine for plain objects, but for anything whose state lives only in getters,Object.keys()returns[]. The key-iteration loop then vacuously succeeds and two distinct instances are treated as equal, regardless of what they actually contain.Temporal.Duration,RegExp, any class that exposes values through getters — they all hit this. The result is that form updates get dropped silently when a field value changes to a new instance of such a type.The fix is a single guard placed after the key-count check:
When both objects have zero own enumerable keys and at least one isn't a plain
{}, they fall back to referential inequality (Object.isat the top already returnedfalse). Arrays get an explicit carve-out soevaluate([], [])still returnstrue.PR #1939 adds a specific
instanceof Blobguard forFile/Blob. This guard is more general and covers the same case, plusTemporal,RegExp,Error, and anything else with a getter-only design. If #1939 merges first, itsinstanceof Blobcheck fires before reaching this one, so there's no conflict. If this merges first,File/Blobis already covered.One thing to flag: two
RegExpliterals with identical source and flags (like/abc/gvs/abc/g) now returnfalse, since they're different instances. Previously they returnedtruevacuously, which was also wrong: it suppressed updates when a regex field actually changed. The behavior is now in the right direction. A dedicatedinstanceof RegExphandler (like the existingDateone) could add semantic equality if that turns out to be useful.✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes