Skip to content

Comments

Allow undefined fields on JsonObject#1109

Open
skiplabsdaniel wants to merge 1 commit intoSkipLabs:mainfrom
skiplabsdaniel:allow-undefined
Open

Allow undefined fields on JsonObject#1109
skiplabsdaniel wants to merge 1 commit intoSkipLabs:mainfrom
skiplabsdaniel:allow-undefined

Conversation

@skiplabsdaniel
Copy link
Contributor

@skiplabsdaniel skiplabsdaniel commented Feb 20, 2026

Today, defining an object with optional values involves the systematic use of the null value or defining the union of all versions of the type.

Copy link
Contributor

@mbouaziz mbouaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Note: this review was written entirely by me (Claude Code), not by Mehdi.

The defaultParamEncoder fix is correct, but widening the core JsonObject type has ripple effects that aren't addressed here.

Concerns with adding undefined to JsonObject

  • deepFreeze() iterates Object.values() and will throw on undefined values since it doesn't handle that type.
  • JSON.stringify() silently drops undefinedObjectHandle.toString() uses JSON.stringify(this.toJSON()), so undefined properties would be silently omitted from serialization, making roundtrips lossy.
  • exportJSON() converts undefined → CJSON null — meaning undefined values become null when exported to the Skip runtime. This is a silent lossy conversion.
  • clone()Object.entries() skips undefined values, so cloning a JsonObject with undefined properties would drop them.

Only one consumer is updated

The PR updates defaultParamEncoder but other places that iterate over JsonObject values would need similar guards or would silently misbehave.

Suggestion

If the goal is to support optional fields, a narrower approach (e.g. a separate PartialJsonObject type for specific use cases) might be safer than changing the foundational type the entire runtime relies on.

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