Skip to content

Update JsValue::to_json to support undefined#4212

Merged
jedel1043 merged 3 commits intoboa-dev:mainfrom
jamesthurley:to-json-undefined
Mar 20, 2025
Merged

Update JsValue::to_json to support undefined#4212
jedel1043 merged 3 commits intoboa-dev:mainfrom
jamesthurley:to-json-undefined

Conversation

@jamesthurley
Copy link
Contributor

This Pull Request fixes/closes #3842.

Personally I've found JsValue::to_json to be fairly useless in its current implementation, as it will panic if the data structure being converted contains undefined, and I often don't have control over this.

The PR changes the following:

The JsValue::to_json method no longer panics if the structure contains undefined.

If undefined is encountered it will:

  • Omit the property if it is part of an object.
  • Replace with null if undefined is an element of an array. I don't love this, but couldn't think of a better solution.
  • Return None if to_json is being called on JsValue::undefined().

Because of the last point there is a breaking change: JsValue::to_json now returns JsResult<Option<Value>> instead of JsResult<Value>.

To avoid a breaking change we could perhaps introduce a new method try_to_json instead, and have the original to_json call try_to_json and panic if None is returned.

On the other hand, the new behaviour forces people to think about handling undefined, which will probably prevent unexpected panics in the future.

@codecov
Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 52.59%. Comparing base (6ddc2b4) to head (882fb4e).
Report is 396 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/value/conversions/serde_json.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4212      +/-   ##
==========================================
+ Coverage   47.24%   52.59%   +5.34%     
==========================================
  Files         476      487      +11     
  Lines       46892    51965    +5073     
==========================================
+ Hits        22154    27330    +5176     
+ Misses      24738    24635     -103     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jedel1043
Copy link
Member

Replace with null if undefined is an element of an array. I don't love this, but couldn't think of a better solution.

I think this is fine. The specification already does this, so it is nice to have parity with it.

@jedel1043 jedel1043 requested a review from a team March 20, 2025 15:56
@jedel1043 jedel1043 added A-Enhancement New feature or request A-API Changes related to public APIs labels Mar 20, 2025
@jedel1043 jedel1043 added this to the next-release milestone Mar 20, 2025
Comment on lines 116 to 118
/// # Panics
///
/// Panics if the `JsValue` is `Undefined`.
Copy link
Member

@jedel1043 jedel1043 Mar 20, 2025

Choose a reason for hiding this comment

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

This should probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good catch.

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the contribution!

@jedel1043 jedel1043 added this pull request to the merge queue Mar 20, 2025
Merged via the queue into boa-dev:main with commit 6f03c21 Mar 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-API Changes related to public APIs A-Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

to_json() on a nested data structure containing undefined should not fail

2 participants