fix(types): Optional JSON params where undefined is not valid

[legobeat]

Follow-up to #130 (#129).

This should now:

• Not specify undefined as valid JSON value in types
• Keep params field optional in types
• Preserve backwards-compatability

The hardcoded types were extracted from the resulting json.d.ts after a build of v8.0.0 (current main), with the only change in that params: was changed into params?:. This seems to be necessary due to the way superstruct omit and optional work.

[legobeat]

Full diff of resulting built package and typings compared to v8.0.0:

CJS

diff -x '*.map' -wr -U1 dist-v8.0.0/cjs/json.js dist-728519a3/cjs/json.js
--- dist-v8.0.0/cjs/json.js
+++ dist-728519a3/cjs/json.js
@@ -161,6 +161,6 @@
 });
-const JsonRpcParamsStruct = (0, _superstruct.optional)((0, _superstruct.union)([
+const JsonRpcParamsStruct = (0, _superstruct.union)([
     (0, _superstruct.record)((0, _superstruct.string)(), JsonStruct),
     (0, _superstruct.array)(JsonStruct)
-]));
+]);
 const JsonRpcRequestStruct = (0, _superstruct.object)({
@@ -169,7 +169,9 @@
     method: (0, _superstruct.string)(),
-    params: JsonRpcParamsStruct
+    params: (0, _superstruct.optional)(JsonRpcParamsStruct)
+});
+const JsonRpcNotificationStruct = (0, _superstruct.object)({
+    jsonrpc: JsonRpcVersionStruct,
+    method: (0, _superstruct.string)(),
+    params: (0, _superstruct.optional)(JsonRpcParamsStruct)
 });
-const JsonRpcNotificationStruct = (0, _superstruct.omit)(JsonRpcRequestStruct, [
-    'id'
-]);
 function isJsonRpcNotification(value) {

ESM

diff -x '*.map' -wr -U1 dist-v8.0.0/esm/json.js dist-728519a3/esm/json.js
--- dist-v8.0.0/esm/json.js
+++ dist-728519a3/esm/json.js
@@ -1,2 +1,2 @@
-import { any, array, boolean, coerce, create, define, integer, is, lazy, literal, nullable, number, object, omit, optional, record, string, union, unknown } from 'superstruct';
+import { any, array, boolean, coerce, create, define, integer, is, lazy, literal, nullable, number, object, optional, record, string, union, unknown } from 'superstruct';
 import { assertStruct } from './assert';
@@ -90,6 +90,6 @@
 });
-export const JsonRpcParamsStruct = optional(union([
+export const JsonRpcParamsStruct = union([
     record(string(), JsonStruct),
     array(JsonStruct)
-]));
+]);
 export const JsonRpcRequestStruct = object({
@@ -98,7 +98,9 @@
     method: string(),
-    params: JsonRpcParamsStruct
+    params: optional(JsonRpcParamsStruct)
+});
+export const JsonRpcNotificationStruct = object({
+    jsonrpc: JsonRpcVersionStruct,
+    method: string(),
+    params: optional(JsonRpcParamsStruct)
 });
-export const JsonRpcNotificationStruct = omit(JsonRpcRequestStruct, [
-    'id'
-]);
 /**

Typings

diff -x '*.map' -wr -U1 dist-v8.0.0/types/json.d.ts dist-728519a3/types/json.d.ts
--- dist-v8.0.0/types/json.d.ts
+++ dist-728519a3/types/json.d.ts
@@ -94,10 +94,10 @@
     id: string | number | null;
+    jsonrpc: '2.0';
     method: string;
-    jsonrpc: "2.0";
-    params: Json[] | Record<string, Json>;
+    params?: Json[] | Record<string, Json>;
 }, {
     id: Struct<string | number | null, null>;
-    jsonrpc: Struct<"2.0", "2.0">;
+    jsonrpc: Struct<'2.0', '2.0'>;
     method: Struct<string, null>;
-    params: Struct<Json[] | Record<string, Json>, null>;
+    params?: Struct<Json[] | Record<string, Json>, null>;
 }>;
@@ -113,11 +113,10 @@
 export declare const JsonRpcNotificationStruct: Struct<{
+    jsonrpc: '2.0';
     method: string;
-    jsonrpc: "2.0";
-    params: Json[] | Record<string, Json>;
-}, Omit<{
-    id: Struct<string | number | null, null>;
-    jsonrpc: Struct<"2.0", "2.0">;
+    params?: Json[] | Record<string, Json>;
+}, {
+    jsonrpc: Struct<'2.0', '2.0'>;
     method: Struct<string, null>;
-    params: Struct<Json[] | Record<string, Json>, null>;
-}, "id">>;
+    params?: Struct<Json[] | Record<string, Json>, null>;
+}>;
 /**

[mcmire]

I am a bit worried that this will cause repercussions that we might not have accounted for yet. We should see if we can test these changes in core, Snaps, and perhaps even other libraries like json-rpc-engine or eth-json-rpc-middleware first. I don't know if I will have time to look into this right now, just want to advise that we might want to tread carefully here.

[legobeat]

@mcmire I've patched in the built version of this PR in packages you mentioned here:

• json-rpc-engine: DONOTMERGE: bump @metamask/rpc-errors to 5.1.1 patched with MetaMask/rpc-errors/pull/102 json-rpc-engine#161
• eth-json-rpc-middleware: TEST: Patch @metamask/utils v8.0.0 with https://github.com/MetaMask/utils/pull/134 eth-json-rpc-middleware#238
• core: TEST: @metamask/utils patch c8306e56f6c11c7f18323868f7995a95ab8202e1 core#1640
• snaps: TEST: Patch @metamask/utils v8.0.0 with https://github.com/MetaMask/utils/pull/134 snaps#1705
• rpc-errors: [DONOTMERGE]: Validate MetaMask/utils#134 rpc-errors#104

@metamask/rpc-errors depends on this package and their types are interrelated.

[mcmire]

Ah nice, thank you. I see CI passes in each case (i.e. no type errors). That's a good sign at least!

[legobeat]

@mcmire Updated above comment with eth-json-rpc-middleware and snaps.

[legobeat]

See #135 - a less hacky alternative. Merging it into this one.

Thank you @Mrtenz for making the final (:crossed_fingers:) implementation :1st_place_medal:

[legobeat]

Updated external tests (from #135 (comment)):

• TEST: @metamask/utils patch a00dfc9d9bfcc3215161124d9b1f4dd6009e831c  core#1641
• [DONOTMERGE]: Validate MetaMask/utils#135 rpc-errors#105
• TEST: Patch @metamask/utils v8.0.0 with https://github.com/MetaMask/utils/pull/135 eth-json-rpc-middleware#239
• DONOTMERGE: bump @metamask/rpc-errors to 5.1.1 patched with MetaMask/rpc-errors/pull/102 (utils #135) json-rpc-engine#163

Additional:

• patch @metamask/utils a00dfc9d9bfcc3215161124d9b1f4dd6009e831c keyring-api#89
• TEST: Patch @metamask/utils v8.0.0 with https://github.com/MetaMask/utils/pull/135 snaps#1711