Skip to content

Remove Dependency on Newtonsoft.Json from JsonRpc#1201

Merged
AArnott merged 1 commit intomicrosoft:mainfrom
eerhardt:RemoveJsonConvert
Jun 14, 2025
Merged

Remove Dependency on Newtonsoft.Json from JsonRpc#1201
AArnott merged 1 commit intomicrosoft:mainfrom
eerhardt:RemoveJsonConvert

Conversation

@eerhardt
Copy link
Copy Markdown
Member

The call to JsonConvert from JsonRpc.InvokeCoreAsync is unconditionally preserving Newtonsoft.Json in trimmed/aot'd apps.

Inspecting the current code, I am not sure how this error case ever happens. The call to InvokeCoreAsync only returns JsonRpcError or JsonRpcResult objects. The else shouldn't ever be taken.

Instead of serializing the object using Newtonsoft.Json, simply put the object's Type into the exception message to indicate what the unexpected object was.

cc @AArnott

The call to JsonConvert from JsonRpc.InvokeCoreAsync is unconditionally preserving Newtonsoft.Json in trimmed/aot'd apps.

Inspecting the current code, I am not sure how this error case ever happens. The call to InvokeCoreAsync only returns JsonRpcError or JsonRpcResult objects. The else shouldn't ever be taken.

Instead of serializing the object using Newtonsoft.Json, simply put the object's Type into the exception message to indicate what the unexpected object was.
Copy link
Copy Markdown
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

/azp run

@AArnott AArnott enabled auto-merge June 13, 2025 22:32
Copy link
Copy Markdown
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

/azp run

Copy link
Copy Markdown
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

/azp run

@AArnott
Copy link
Copy Markdown
Member

AArnott commented Jun 14, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

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