[json-rpc-engine] Fix any types in catch blocks, refactor #3906

MajorLift · 2024-02-07T21:28:36Z

Explanation

Fixes all any usage in the JsonRpcEngine class.
Errors are typed consistently as unknown to reflect actual usage in consumers and tests. - JsonRpcEngineCallbackError is no longer used within the package.

References

Closes json-rpc-engine: Replace use of any with proper types (non-test files only) #3720

Changelog

`@metamask/json-rpc-engine`

BREAKING: The type of the error parameters in JsonRpcEngineReturnHandler, JsonRpcEngineNextCallback, and JsonRpcEngineEndCallback are widened from JsonRpcEngineCallbackError to unknown.

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

mcmire

I left some minor comments but overall this looks good. Would be great to address these type issues as they seem to be multiplying, but that seems like another can of worms 😅

packages/json-rpc-engine/src/JsonRpcEngine.ts

packages/json-rpc-engine/src/createAsyncMiddleware.ts

mcmire · 2024-03-14T19:41:14Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

-            return handlerCallback(error);
+          } catch (error) {
+            // TODO: Explicitly handle errors thrown from `#runReturnHandlers` that are not of type `JsonRpcEngineCallbackError`
+            return handlerCallback(error as JsonRpcEngineCallbackError);


Out of interest, do we need to change the type of the error argument? That seems to necessitate adding a type assertion here, so we'd now be lying about the type of this error. I just want to make sure: do we think that this is preferable to using any?

On second glance, the error is being thrown by #runReturnHandlers, which is guaranteed to throw SerializableJsonRpcError | Error. Removed the assertion and added a comment explaining why: 4b61631

All of the other type assertions with TODOs are end() calls.

With these, I think it doesn't make sense to inaccurately force error into the JsonRpcEngineCallbackError type when the test specs are passing in all error objects thrown by middleware into end callbacks regardless of their type.

IMO whatever type safety gains we get from narrowing the error type from unknown are cancelled out by the fact that we have to use inaccurate type assertions everywhere to make the narrower type constraint work.

I widened JsonRpcEngineCallback's error param to unknown, which lets us remove the TODO comments and the type assertions. What do you think? Would this be a regression in terms of type safety or too disruptive to downstream code?
bac8e0e b9549ed

I think these changes look better overall and make more sense than the previous changes.

packages/json-rpc-engine/src/JsonRpcEngine.ts

packages/json-rpc-engine/src/createAsyncMiddleware.ts

packages/json-rpc-engine/jest.config.js

…lbackError`

…own` and remove now unnecessary narrowing branches

MajorLift

I attempted a new approach of replacing all JsonRpcEngineCallbackError with unknown.

This cuts down on the diffs, makes a lot of the types consistent with how they're used in the tests, makes a lot of assertions unnecessary, and brings coverage back up to 100%.

I think this might be an appropriate scope for the current PR's objective (fix any). Implementing consistent usage of JsonRpcEngineCallbackError for the middleware callbacks should probably be handled in its own initiative.

MajorLift · 2024-03-20T04:24:43Z

packages/json-rpc-engine/src/JsonRpcEngine.ts


 export type JsonRpcEngineCallbackError = Error | SerializedJsonRpcError | null;

 export type JsonRpcEngineReturnHandler = (
-  done: (error?: JsonRpcEngineCallbackError) => void,
+  done: (error?: unknown) => void,


New strategy: consistently type all errors as unknown.

MajorLift · 2024-03-20T04:24:46Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

-export type JsonRpcEngineEndCallback = (
-  error?: JsonRpcEngineCallbackError,
-) => void;
+export type JsonRpcEngineEndCallback = (error?: unknown) => void;


New strategy: consistently type all errors as unknown.

MajorLift · 2024-03-20T04:24:58Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

+  hasProperty,
+  isJsonRpcNotification,
+  isJsonRpcRequest,
+} from '@metamask/utils';

 export type JsonRpcEngineCallbackError = Error | SerializedJsonRpcError | null;


JsonRpcEngineCallbackError is no longer used anywhere in core or extension. Mobile has two instances in RPCMethodMiddleware.ts.

MajorLift · 2024-03-20T04:25:10Z

packages/json-rpc-engine/src/createAsyncMiddleware.ts

@@ -18,7 +21,7 @@ export type AsyncJsonrpcMiddleware<
  next: AsyncJsonRpcEngineNextCallback,
 ) => Promise<void>;

-type ReturnHandlerCallback = (error: null | Error) => void;
+type ReturnHandlerCallback = Parameters<JsonRpcEngineReturnHandler>[0];


This definition is responsive to changes in JsonRpcEngineReturnHandler.

MajorLift · 2024-03-20T04:25:25Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

@@ -317,8 +328,8 @@ export class JsonRpcEngine extends SafeEventEmitter {
        )
      ).filter(
        // Filter out any notification responses.
-        (response) => response !== undefined,
-      ) as JsonRpcResponse<Json>[];
+        (response): response is JsonRpcResponse<Json> => response !== undefined,


See https://github.com/MetaMask/contributor-docs/blob/240206-typescript-guidelines-any-as/docs/typescript.md#example-f7ff4b0d-e5e9-4568-b916-5153ddd2095b

Nice! Good example usage of this.

MajorLift · 2024-03-20T04:25:28Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

+      | (JsonRpcRequest | JsonRpcNotification)[]
+      | JsonRpcRequest
+      | JsonRpcNotification,
+    callback?: (error: unknown, response: never) => void,


never is used to widen the implementation signature enough to be compatible with all of the overload signatures.

MajorLift · 2024-03-20T04:28:12Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

+      isJsonRpcNotification(callerReq) &&
+      !isJsonRpcRequest(callerReq)


isJsonRpcNotification alone doesn't narrow callerReq to JsonRpcNotification.

MajorLift · 2024-03-20T16:14:56Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

+          // This assertion is safe because of the runtime checks validating that `req` is an array and `callback` is defined.
+          // There is only one overload signature that satisfies both conditions, and its `callback` type is the one that's being asserted.
+          callback as (
+            error: unknown,
+            responses?: JsonRpcResponse<Json>[],
+          ) => void,


Ideally, TypeScript would have better pattern-matching around overload signatures and would be capable of narrowing callback and responses so that this assertion is unnecessary.

MajorLift · 2024-03-20T16:16:21Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

-      return this.#handle(req as JsonRpcRequest, callback);
+      return this.#handle(
+        req,
+        callback as (error: unknown, response?: JsonRpcResponse<Json>) => void,


The first and second overload signatures of handle are the only cases where req is a non-array and callback is defined. This is assertion is safe for those overload signatures.

mcmire

I just had one more comment, but this otherwise looks good to me.

mcmire · 2024-03-20T20:00:36Z

packages/json-rpc-engine/src/JsonRpcEngine.ts

        const parsedError = error || response.error;
        if (parsedError) {
          response.error = serializeError(parsedError);
        }
        // True indicates that the request should end
-        resolve([parsedError, true]);
+        resolve([parsedError ?? null, true]);


It seems that we are converting undefined to null here. This could matter, so what do you think about this:

Suggested change

const parsedError = error || response.error;

if (parsedError) {

response.error = serializeError(parsedError);

}

// True indicates that the request should end

resolve([parsedError, true]);

resolve([parsedError ?? null, true]);

const parsedError: unknown = error || response.error;

if (parsedError) {

response.error = serializeError(parsedError);

}

// True indicates that the request should end

resolve([parsedError, true]);

Thanks for catching this. It's no longer necessary now that JsonRpcEngineCallbackError is not being used here.

Applied here: cfaa18b, except I didn't add the : unknown annotation because {} is NonNullable<unknown>, meaning unknown would pollute parsedError's type signature with a null that previously wasn't there.

…idened to `unknown`

mcmire

Everything looks good to me now. Nice work!

MajorLift self-assigned this Feb 7, 2024

MajorLift added the team-wallet-framework label Feb 7, 2024

MajorLift force-pushed the 230205-fix-any-json-rpc-engine branch from 70119ee to c59d1b1 Compare February 7, 2024 21:33

MajorLift force-pushed the 230205-fix-any-json-rpc-engine branch 3 times, most recently from ca41d02 to 58949ad Compare March 7, 2024 03:44

MajorLift marked this pull request as ready for review March 7, 2024 03:51

MajorLift requested a review from a team as a code owner March 7, 2024 03:51

MajorLift added 3 commits March 12, 2024 21:21

Fix any in middleware error handling

af6ffbc

Fix any in json-rpc request error handling

49327db

Fix any in createAsyncMiddleware

c81e0cc

MajorLift force-pushed the 230205-fix-any-json-rpc-engine branch from 28b21db to c81e0cc Compare March 13, 2024 01:21

mcmire reviewed Mar 13, 2024

View reviewed changes

packages/json-rpc-engine/src/JsonRpcEngine.ts Outdated Show resolved Hide resolved

packages/json-rpc-engine/src/createAsyncMiddleware.ts Outdated Show resolved Hide resolved

MajorLift added 4 commits March 14, 2024 13:37

Remove unnecessary error: unknown annotations in catch blocks

6f9f1e2

Revert #handleRequest refactor

8854416

Remove isNativeError for browser compatibility

8d2a9d1

Merge branch 'main' into 230205-fix-any-json-rpc-engine

145570f

mcmire reviewed Mar 14, 2024

View reviewed changes

MajorLift added 3 commits March 14, 2024 16:13

Remove unnecessary assertion where thrown error type is known

4b61631

Widen JsonRpcEngineEndCallback error param type to unknown

bac8e0e

Remove TODOs for typing error passed into end callbacks

b9549ed

mcmire reviewed Mar 15, 2024

View reviewed changes

packages/json-rpc-engine/src/JsonRpcEngine.ts Outdated Show resolved Hide resolved

packages/json-rpc-engine/src/createAsyncMiddleware.ts Outdated Show resolved Hide resolved

packages/json-rpc-engine/jest.config.js Outdated Show resolved Hide resolved

MajorLift added 4 commits March 19, 2024 13:41

linter fix

4c631d2

Consistently use unknown as error type instead of `JsonRpcEngineCal…

950237e

…lbackError`

Remove erroneous type assertions of request objects to Json type

77b2c92

Remove or fix unnecessary type assertions

ed282d8

MajorLift force-pushed the 230205-fix-any-json-rpc-engine branch from 1df21c3 to ed282d8 Compare March 19, 2024 17:41

Widen JsonRpcEngineReturnHandler callback param error type to `unkn…

45a318e

…own` and remove now unnecessary narrowing branches

MajorLift commented Mar 20, 2024

View reviewed changes

Fix optional error param to required

e8f7892

Add comment documenting safe assertion

b1b3d1b

MajorLift commented Mar 20, 2024

View reviewed changes

Revert code removed in error

97214c2

mcmire reviewed Mar 20, 2024

View reviewed changes

MajorLift added 2 commits March 20, 2024 16:07

Remove fallback to null that's now unnecessary due to error being w…

cfaa18b

…idened to `unknown`

Merge branch 'main' into 230205-fix-any-json-rpc-engine

b8bbdaa

mcmire approved these changes Mar 20, 2024

View reviewed changes

MajorLift merged commit 0c78d6d into main Mar 20, 2024
139 checks passed

MajorLift deleted the 230205-fix-any-json-rpc-engine branch March 20, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[json-rpc-engine] Fix any types in catch blocks, refactor #3906

[json-rpc-engine] Fix any types in catch blocks, refactor #3906

MajorLift commented Feb 7, 2024 •

edited

Loading

mcmire left a comment

mcmire Mar 14, 2024 •

edited

Loading

MajorLift Mar 14, 2024

MajorLift Mar 15, 2024 •

edited

Loading

mcmire Mar 15, 2024

MajorLift left a comment •

edited

Loading

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024 •

edited

Loading

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024

mcmire Mar 20, 2024

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024

MajorLift Mar 20, 2024

mcmire left a comment

mcmire Mar 20, 2024

MajorLift Mar 20, 2024 •

edited

Loading

mcmire left a comment

		isJsonRpcNotification(callerReq) &&
		!isJsonRpcRequest(callerReq)

[json-rpc-engine] Fix any types in catch blocks, refactor #3906

[json-rpc-engine] Fix any types in catch blocks, refactor #3906

Conversation

MajorLift commented Feb 7, 2024 • edited Loading

Explanation

References

Changelog

@metamask/json-rpc-engine

Checklist

mcmire left a comment

Choose a reason for hiding this comment

mcmire Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

MajorLift commented Feb 7, 2024 •

edited

Loading

`@metamask/json-rpc-engine`

mcmire Mar 14, 2024 •

edited

Loading

MajorLift Mar 15, 2024 •

edited

Loading

MajorLift left a comment •

edited

Loading

MajorLift Mar 20, 2024 •

edited

Loading

MajorLift Mar 20, 2024 •

edited

Loading