-
-
Notifications
You must be signed in to change notification settings - Fork 740
Fix API break: public API must not expose JsonElement objects #938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This changes deserialization to the way how it was with Json.Net: Only .net primitive types are exposed publicly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses an API breaking change by ensuring the public API returns .NET primitive types instead of exposing JsonElement objects directly, restoring compatibility with the previous Json.NET-based behavior.
Key Changes:
- Introduced a new
JsonToBoxedPrimitivesConverterthat deserializes JSON to boxed primitive types (int, long, double, string, bool, etc.) - Refactored
IpcMain.csto use the new converter and simplified argument handling to extract the actual payload from IPC messages - Updated integration tests to verify that received objects are primitive types rather than JsonElement instances
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/ElectronNET.API/Serialization/JsonToBoxedPrimitivesConverter.cs |
New JSON converter that deserializes to boxed primitive .NET types instead of JsonElement objects |
src/ElectronNET.API/API/IpcMain.cs |
Refactored to use new converter and simplified FormatArguments to extract payload from IPC message arrays |
src/ElectronNET.IntegrationTests/Tests/IpcMainTests.cs |
Added assertions to verify received objects are primitive string types, validating the API fix |
src/ElectronNET.IntegrationTests/ElectronNET.IntegrationTests.csproj |
Disabled nullable reference types to simplify test code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@Denny09310 et al.The serialization changes broke my application. It uses IPC for sending plain strings back and forth to the browser. After the change to System.Text.Json, it started to receive JsonElement objects instead of strings. The reason for this is that Json.Net fully deserializes everything to .net primitives, but S.T.J still gives you JsonElement objects - e.g. when you deserialize to I have created a converter which should be close to Json.NET behavior (but there might be tiny differences). We need to check for other cases......where this specific difference might hit in. Affected are all cases where there's a deserialization to |
d83249c to
be518a7
Compare
FlorianRappl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, LGTM!
|
Should we keep it open for now or have it in a preview release to invite others to check for differences? In general I see this as a 0.3.0 release. I'll make the changes to |
|
A preview release would be great so that I don't need to revert to our private packages :-) There are definitely more cases. (I did a search for |
This changes deserialization to the way how it was with
Json.Net: Only .net primitive types are exposed publicly