Skip to content

Conversation

@Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Jan 5, 2026

According to the JSON RPC spec:

5 Response object

When a rpc call is made, the Server MUST reply with a Response, except for in the case of Notifications. The Response is expressed as a single JSON Object, with the following members:

jsonrpc
A String specifying the version of the JSON-RPC protocol. MUST be exactly "2.0".
result
This member is REQUIRED on success.
This member MUST NOT exist if there was an error invoking the method.
The value of this member is determined by the method invoked on the Server.
error
This member is REQUIRED on error.
This member MUST NOT exist if there was no error triggered during invocation.
The value for this member MUST be an Object as defined in section 5.1.
id
This member is REQUIRED.
It MUST be the same as the value of the id member in the Request Object.
If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null.
Either the result member or error member MUST be included, but both members MUST NOT be included.

From this we can conclude that the response MUST exist if there is not an error property. I've updated src/SIL.XForge.Scripture/ClientApp/src/xforge-common/command.service.ts to encode this logic, and this allows a lot of non-null assertions, or null checks, to be removed elsewhere. Without this type change, every RPC call requires us to check whether the return value exists (if we care about the value), even though it's guaranteed to exist.


This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (3009c3b) to head (6f57267).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...pture/ClientApp/src/app/core/sf-project.service.ts 22.22% 7 Missing ⚠️
...c/app/event-metrics/event-metrics-log.component.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3622   +/-   ##
=======================================
  Coverage   82.82%   82.82%           
=======================================
  Files         609      609           
  Lines       37426    37426           
  Branches     6135     6159   +24     
=======================================
  Hits        30998    30998           
+ Misses       5495     5481   -14     
- Partials      933      947   +14     

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

Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

:lgtm:

@RaymondLuong3 reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami).

@RaymondLuong3 RaymondLuong3 merged commit 185682d into master Jan 9, 2026
23 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/rpc-types branch January 9, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants