Skip to content
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

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

Merged
merged 20 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
af6ffbc
Fix `any` in middleware error handling
MajorLift Mar 7, 2024
49327db
Fix `any` in json-rpc request error handling
MajorLift Mar 7, 2024
c81e0cc
Fix any in `createAsyncMiddleware`
MajorLift Mar 13, 2024
6f9f1e2
Remove unnecessary `error: unknown` annotations in catch blocks
MajorLift Mar 14, 2024
8854416
Revert `#handleRequest` refactor
MajorLift Mar 14, 2024
8d2a9d1
Remove `isNativeError` for browser compatibility
MajorLift Mar 14, 2024
145570f
Merge branch 'main' into 230205-fix-any-json-rpc-engine
MajorLift Mar 14, 2024
4b61631
Remove unnecessary assertion where thrown error type is known
MajorLift Mar 14, 2024
bac8e0e
Widen `JsonRpcEngineEndCallback` `error` param type to `unknown`
MajorLift Mar 14, 2024
b9549ed
Remove TODOs for typing `error` passed into `end` callbacks
MajorLift Mar 14, 2024
4c631d2
linter fix
MajorLift Mar 14, 2024
950237e
Consistently use `unknown` as error type instead of `JsonRpcEngineCal…
MajorLift Mar 19, 2024
77b2c92
Remove erroneous type assertions of request objects to `Json` type
MajorLift Mar 19, 2024
ed282d8
Remove or fix unnecessary type assertions
MajorLift Mar 19, 2024
45a318e
Widen `JsonRpcEngineReturnHandler` callback param error type to `unkn…
MajorLift Mar 20, 2024
e8f7892
Fix optional `error` param to required
MajorLift Mar 20, 2024
b1b3d1b
Add comment documenting safe assertion
MajorLift Mar 20, 2024
97214c2
Revert code removed in error
MajorLift Mar 20, 2024
cfaa18b
Remove fallback to null that's now unnecessary due to `error` being w…
MajorLift Mar 20, 2024
b8bbdaa
Merge branch 'main' into 230205-fix-any-json-rpc-engine
MajorLift Mar 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 46 additions & 41 deletions packages/json-rpc-engine/src/JsonRpcEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,23 @@ import type {
JsonRpcParams,
PendingJsonRpcResponse,
} from '@metamask/utils';
import { hasProperty, isJsonRpcRequest } from '@metamask/utils';
import {
hasProperty,
isJsonRpcNotification,
isJsonRpcRequest,
} from '@metamask/utils';

export type JsonRpcEngineCallbackError = Error | SerializedJsonRpcError | null;
Copy link
Contributor Author

@MajorLift MajorLift Mar 20, 2024

Choose a reason for hiding this comment

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

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


export type JsonRpcEngineReturnHandler = (
done: (error?: JsonRpcEngineCallbackError) => void,
done: (error?: unknown) => void,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New strategy: consistently type all errors as unknown.

) => void;

export type JsonRpcEngineNextCallback = (
returnHandlerCallback?: JsonRpcEngineReturnHandler,
) => void;

export type JsonRpcEngineEndCallback = (
error?: JsonRpcEngineCallbackError,
) => void;
export type JsonRpcEngineEndCallback = (error?: unknown) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New strategy: consistently type all errors as unknown.


export type JsonRpcMiddleware<
Params extends JsonRpcParams,
Expand Down Expand Up @@ -200,9 +202,13 @@ export class JsonRpcEngine extends SafeEventEmitter {
requests: (JsonRpcRequest<Params> | JsonRpcNotification<Params>)[],
): Promise<JsonRpcResponse<Result>[]>;

// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
handle(req: unknown, callback?: any) {
handle(
req:
| (JsonRpcRequest | JsonRpcNotification)[]
| JsonRpcRequest
| JsonRpcNotification,
callback?: (error: unknown, response: never) => void,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

) {
this.#assertIsNotDestroyed();

if (callback && typeof callback !== 'function') {
Expand All @@ -211,15 +217,24 @@ export class JsonRpcEngine extends SafeEventEmitter {

if (Array.isArray(req)) {
if (callback) {
return this.#handleBatch(req, callback);
return this.#handleBatch(
req,
callback as (
error: unknown,
responses?: JsonRpcResponse<Json>[],
) => void,
);
}
return this.#handleBatch(req);
}

if (callback) {
return this.#handle(req as JsonRpcRequest, callback);
return this.#handle(
req,
callback as (error: unknown, response?: JsonRpcResponse<Json>) => void,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

);
}
return this._promiseHandle(req as JsonRpcRequest);
return this._promiseHandle(req);
}

/**
Expand All @@ -239,23 +254,19 @@ export class JsonRpcEngine extends SafeEventEmitter {

if (isComplete) {
await JsonRpcEngine.#runReturnHandlers(returnHandlers);
return end(middlewareError as JsonRpcEngineCallbackError);
return end(middlewareError);
}

// eslint-disable-next-line @typescript-eslint/no-misused-promises
return next(async (handlerCallback) => {
try {
await JsonRpcEngine.#runReturnHandlers(returnHandlers);
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
} catch (error) {
return handlerCallback(error);
}
return handlerCallback();
});
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
} catch (error) {
return end(error);
}
};
Expand Down Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Good example usage of this.

);

// 3. Return batch response
if (callback) {
Expand Down Expand Up @@ -392,7 +403,7 @@ export class JsonRpcEngine extends SafeEventEmitter {
const error = new JsonRpcError(
errorCodes.rpc.invalidRequest,
`Must specify a string method. Received: ${typeof callerReq.method}`,
{ request: callerReq as Json },
{ request: callerReq },
);

if (this.#notificationHandler && !isJsonRpcRequest(callerReq)) {
Expand All @@ -403,25 +414,23 @@ export class JsonRpcEngine extends SafeEventEmitter {
return callback(error, {
// Typecast: This could be a notification, but we want to access the
// `id` even if it doesn't exist.
id: (callerReq as JsonRpcRequest).id ?? null,
id: (callerReq as JsonRpcRequest).id,
jsonrpc: '2.0',
error,
});
}

// Handle notifications.
// We can't use isJsonRpcNotification here because that narrows callerReq to
// "never" after the if clause for unknown reasons.
if (this.#notificationHandler && !isJsonRpcRequest(callerReq)) {
} else if (
this.#notificationHandler &&
isJsonRpcNotification(callerReq) &&
!isJsonRpcRequest(callerReq)
Comment on lines +425 to +426
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isJsonRpcNotification alone doesn't narrow callerReq to JsonRpcNotification.

) {
try {
await this.#notificationHandler(callerReq);
} catch (error) {
return callback(error);
}
return callback(null);
}

let error: JsonRpcEngineCallbackError = null;
let error = null;

// Handle requests.
// Typecast: Permit missing id's for backwards compatibility.
Expand All @@ -433,9 +442,7 @@ export class JsonRpcEngine extends SafeEventEmitter {

try {
await JsonRpcEngine.#processRequest(req, res, this.#middleware);
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (_error: any) {
} catch (_error) {
// A request handler error, a re-thrown middleware error, or something
// unexpected.
error = _error;
Expand Down Expand Up @@ -543,13 +550,13 @@ export class JsonRpcEngine extends SafeEventEmitter {
returnHandlers: JsonRpcEngineReturnHandler[],
): Promise<[unknown, boolean]> {
return new Promise((resolve) => {
const end: JsonRpcEngineEndCallback = (error?: unknown) => {
const end: JsonRpcEngineEndCallback = (error) => {
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]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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]);

Copy link
Contributor Author

@MajorLift MajorLift Mar 20, 2024

Choose a reason for hiding this comment

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

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.

};

const next: JsonRpcEngineNextCallback = (
Expand All @@ -567,7 +574,7 @@ export class JsonRpcEngine extends SafeEventEmitter {
`Received "${typeof returnHandler}" for request:\n${jsonify(
request,
)}`,
{ request: request as Json },
{ request },
),
);
}
Expand All @@ -581,9 +588,7 @@ export class JsonRpcEngine extends SafeEventEmitter {

try {
middleware(request, response, next, end);
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
} catch (error) {
end(error);
}
});
Expand Down Expand Up @@ -625,15 +630,15 @@ export class JsonRpcEngine extends SafeEventEmitter {
`JsonRpcEngine: Response has no error or result for request:\n${jsonify(
request,
)}`,
{ request: request as Json },
{ request },
);
}

if (!isComplete) {
throw new JsonRpcError(
errorCodes.rpc.internal,
`JsonRpcEngine: Nothing ended request:\n${jsonify(request)}`,
{ request: request as Json },
{ request },
);
}
}
Expand Down
11 changes: 6 additions & 5 deletions packages/json-rpc-engine/src/createAsyncMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import type {
PendingJsonRpcResponse,
} from '@metamask/utils';

import type { JsonRpcMiddleware } from './JsonRpcEngine';
import type {
JsonRpcEngineReturnHandler,
JsonRpcMiddleware,
} from './JsonRpcEngine';

export type AsyncJsonRpcEngineNextCallback = () => Promise<void>;

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

type ReturnHandlerCallback = (error: null | Error) => void;
type ReturnHandlerCallback = Parameters<JsonRpcEngineReturnHandler>[0];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This definition is responsive to changes in JsonRpcEngineReturnHandler.


/**
* JsonRpcEngine only accepts callback-based middleware directly.
Expand Down Expand Up @@ -83,9 +86,7 @@ export function createAsyncMiddleware<
} else {
end(null);
}
// TODO: Replace `any` with type
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
} catch (error) {
if (returnHandlerCallback) {
(returnHandlerCallback as ReturnHandlerCallback)(error);
} else {
Expand Down
Loading