Fix format() for transformed non-primitive values in @optique/zod and @optique/valibot#706
Fix format() for transformed non-primitive values in @optique/zod and @optique/valibot#706
format() for transformed non-primitive values in @optique/zod and @optique/valibot#706Conversation
6faa53b to
7e23ebd
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the formatting of transformed values in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR makes formatting deterministic for transformed non-primitive values in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of formatting non-primitive values in @optique/zod and @optique/valibot. The new type-aware formatting logic is robust, handling Date objects, plain objects, and various edge cases like non-serializable values correctly. The addition of a custom format option provides valuable flexibility for users. The accompanying tests are comprehensive and ensure the new functionality is well-covered.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6faa53b82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGES.md`:
- Around line 1657-1661: Update the changelog text to reflect actual runtime
behavior: change the phrasing that says arrays use `JSON.stringify()` to instead
state arrays are formatted via `String()` (preserving round-trip behavior like
`split(",")`); adjust both occurrences describing valibot() formatting and the
`format` option in `ValibotParserOptions` so they accurately mention `String()`
for arrays while keeping `Date` -> `.toISOString()` and plain objects ->
`JSON.stringify()` references.
In `@packages/valibot/src/index.test.ts`:
- Around line 385-403: Add a new unit test that constructs an explicit cyclic
object (e.g., const a = {}; a.self = a) and passes it through the same valibot
pipeline used in the neighboring tests (using v.pipe and v.transform to return
the cyclic object), then call parser.format on that cyclic object and assert it
does not throw and returns a safe string (e.g., "[object Object]"); locate the
test near the existing "non-JSON-serializable" cases and reuse the same
parser/format pattern so the cyclic-reference guard is covered.
In `@packages/zod/src/index.test.ts`:
- Around line 387-402: Add a regression test for cyclic objects in the same test
block that checks non-JSON-serializable handling: create a parser using zod
(same pattern as the existing tests using z.string().transform(...)), build a
cyclic object (an object that references itself), call parser.format(cyclicObj)
and assert it does not throw and returns the fallback string (e.g., "[object
Object]") similar to the other assertions; place the test alongside the existing
tests that use the parser variable and zod to ensure the no-throw + fallback
behavior is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 128aa759-80e1-44b1-abcc-2631152184e0
📒 Files selected for processing (5)
CHANGES.mdpackages/valibot/src/index.test.tspackages/valibot/src/index.tspackages/zod/src/index.test.tspackages/zod/src/index.ts
Arrays of objects like [{a:1},{b:2}] produced
"[object Object],[object Object]" because the guard only
checked for exact match with "[object Object]". Now uses
str.includes("[object Object]") to catch object arrays too,
falling through to JSON.stringify for a readable representation.
#285
#706 (comment)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both zod() and valibot() used String(value) for format(), which produced "[object Object]" for transformed object values and locale-dependent strings for Date objects. Now uses type-aware formatting: Date.toISOString() for stable date output, JSON.stringify() for plain objects and arrays, and String() for objects with a meaningful custom toString(). Also adds a format option to ZodParserOptions and ValibotParserOptions for user-provided custom formatters. #285 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Invalid Date instances (e.g., new Date("bad")) would throw
RangeError on toISOString(). Now falls back to String(value)
which returns "Invalid Date" without throwing.
#285
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Objects containing BigInt, cycles, or other non-JSON-serializable values would throw TypeError from JSON.stringify(). Now catches the error and falls back to String(value). #285 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Arrays have a meaningful toString() (e.g., ["a","b"] → "a,b")
that round-trips correctly with common transform patterns like
s.split(","). JSON.stringify would produce "[\"a\",\"b\"]" which
breaks round-trip parsing.
#285
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Objects with toJSON() returning undefined cause JSON.stringify() to return undefined instead of throwing, violating the string return type contract. Now falls back to String(value) in that case using nullish coalescing. #285 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Arrays of objects like [{a:1},{b:2}] produced
"[object Object],[object Object]" because the guard only
checked for exact match with "[object Object]". Now uses
str.includes("[object Object]") to catch object arrays too,
falling through to JSON.stringify for a readable representation.
#285
#706 (comment)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update CHANGES.md to accurately state that arrays use String() (not JSON.stringify()) for round-trip compatibility. Add cyclic object regression tests for both zod and valibot formatters. #285 #706 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fa5882a to
009d3a0
Compare
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable improvement to the formatting of transformed values in both @optique/zod and @optique/valibot, replacing the naive String(value) with more intelligent, type-aware formatting. The new logic correctly handles Date objects, plain objects, and arrays, and the addition of a custom format option provides useful flexibility. The changes are well-tested. While the initial thought was to deduplicate the new formatting logic into a shared utility function in @optique/core, this approach would violate our rule against importing internal helper functions from one package into another if they are not part of the public API, as it compromises package encapsulation. Therefore, the current local implementation in each package is preferred.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/valibot/src/index.ts`:
- Around line 24-28: Update the JSDoc for the `format` parameter/comment to
match the actual fallback behavior: state that primitives are formatted with
String(value), Date objects with value.toISOString(), and that object formatting
will preserve a meaningful String(value) when available and only fall back to
JSON.stringify() for plain/generic objects without a useful toString; adjust the
wording in the `format` JSDoc accordingly so it accurately reflects the
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca4eab0a-745a-42eb-91f0-809c19e2fa4d
📒 Files selected for processing (5)
CHANGES.mdpackages/valibot/src/index.test.tspackages/valibot/src/index.tspackages/zod/src/index.test.tspackages/zod/src/index.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 009d3a034f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace str.includes("[object Object]") with a check against
Object.prototype.toString to decide whether to use String() or
JSON.stringify(). This is more principled and avoids misclassifying
arrays whose string elements literally contain "[object Object]".
Arrays always use String() via Array.prototype.toString (which
differs from Object.prototype.toString), preserving round-trip
compatibility. Plain objects without a custom toString() fall
through to JSON.stringify().
Also updates JSDoc to accurately describe the fallback behavior.
#285
#706 (comment)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an intelligent formatting mechanism for transformed non-primitive values in @optique/zod and @optique/valibot, replacing the naive String(value) implementation. The new logic correctly handles Date objects by using .toISOString(), and plain objects by using JSON.stringify(), with robust fallbacks for non-serializable values. A new format option is also added to allow for custom formatting. The changes are well-implemented, thoroughly tested across various edge cases, and adhere to the project's coding standards. The implementation is solid and I have no further suggestions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e09cb982f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the prototype-based toString check with a combination of Array.isArray (for arrays) and exact String(value) === "[object Object]" comparison (for other objects). This correctly preserves Map, Set, and Symbol.toPrimitive outputs that the prototype check would have routed through JSON.stringify() lossily. #285 #706 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces intelligent formatting for transformed non-primitive values in @optique/zod and @optique/valibot, replacing a naive String(value) implementation. The new logic correctly handles Date objects, plain objects, and arrays, and also adds a format option for custom formatters. The changes are well-tested and address the issue described.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf3fada5bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Use Object.getPrototypeOf to detect actual plain objects (proto
is Object.prototype or null) instead of relying on string output.
Class instances without a custom toString() would have been
routed through JSON.stringify() which drops private/non-enumerable
fields, producing misleading "{}" in help output.
#285
#706 (comment)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the valibot() and zod() parsers by introducing intelligent formatting for transformed non-primitive values, addressing the issue of [object Object] output. Date values are now formatted as ISO strings, plain objects as JSON strings (with graceful fallback for non-serializable or cyclic objects), and arrays as comma-separated strings. Additionally, a new format option has been added to ValibotParserOptions and ZodParserOptions, allowing users to provide custom formatting logic. Comprehensive unit tests have been added to cover these new formatting behaviors and various edge cases. The CHANGES.md file has been updated to document these new features.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bea3df276
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
==========================================
- Coverage 93.96% 93.95% -0.01%
==========================================
Files 38 38
Lines 19134 19169 +35
Branches 5219 5241 +22
==========================================
+ Hits 17979 18010 +31
Misses 1135 1135
- Partials 20 24 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Plain objects with an instance-level toString() (e.g.,
{ raw: s, toString() { return s; } }) were routed through
JSON.stringify() because the prototype check ran first. Now
checks String(value) !== "[object Object]" before the prototype
check, so meaningful toString() results are always preserved.
#285
#706 (comment)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the valibot() and zod() parsers by introducing intelligent default formatting for non-primitive values and adding a new format option for custom formatting. Previously, complex types would default to [object Object]. Now, Date objects are formatted as ISO strings, plain objects are serialized to JSON, and arrays are converted to comma-separated strings. Extensive unit tests have been added to cover various formatting scenarios, including edge cases like invalid dates, non-JSON-serializable objects, and cyclic objects. The CHANGES.md file has been updated to document these new features.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcec46f04b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
Both
zod()andvalibot()usedString(value)as theirformat()implementation, which produced[object Object]for transformed object values and locale/timezone-dependent strings forDateobjects in help and default displays.This PR replaces the naive
String(value)call with type-aware formatting.Dateinstances now use.toISOString()for stable, locale-independent output. Plain objects (those whoseString()representation is[object Object]) fall back toJSON.stringify(), with guards for non-serializable values (e.g.,BigInt, cycles) and edge cases whereJSON.stringify()returnsundefined(e.g., objects with atoJSON()that returnsundefined). Objects and arrays that have a meaningful customtoString()continue to useString()as before, preserving round-trip compatibility with common transform patterns likes.split(",").For example, given a Zod schema
z.string().transform(s => ({ raw: s }))with a default value of{ raw: "x" }, help output previously showed:It now shows:
A new
formatoption has also been added to bothZodParserOptionsandValibotParserOptions, allowing users to provide a custom formatter when the default behavior is not suitable for their use case:The changes are in packages/zod/src/index.ts and packages/valibot/src/index.ts, with corresponding tests in packages/zod/src/index.test.ts and packages/valibot/src/index.test.ts.
Closes #285