From fdd6da93ab409d76da830d24bb47af44fb6b37d3 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 7 Apr 2022 16:26:23 -0700 Subject: [PATCH 1/7] Initial notification handling implementation --- src/{engine.test.ts => JsonRpcEngine.test.ts} | 6 +- src/JsonRpcEngine.ts | 108 +++++++++++++----- 2 files changed, 85 insertions(+), 29 deletions(-) rename src/{engine.test.ts => JsonRpcEngine.test.ts} (98%) diff --git a/src/engine.test.ts b/src/JsonRpcEngine.test.ts similarity index 98% rename from src/engine.test.ts rename to src/JsonRpcEngine.test.ts index 9ea39ea..57731ea 100644 --- a/src/engine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -30,13 +30,13 @@ describe('JsonRpcEngine', () => { it('handle: returns error for invalid request method', async () => { const engine = new JsonRpcEngine(); - let response: any = await engine.handle({ method: null } as any); + let response: any = await engine.handle({ id: 1, method: null } as any); expect(response.error.code).toStrictEqual(-32600); expect(response.result).toBeUndefined(); + // No response if duck-typed as a notification response = await engine.handle({ method: true } as any); - expect(response.error.code).toStrictEqual(-32600); - expect(response.result).toBeUndefined(); + expect(response).toBeUndefined(); }); it('handle: basic middleware test 1', async () => { diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 8f787b4..843ec88 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -4,6 +4,9 @@ import { JsonRpcError, JsonRpcRequest, JsonRpcResponse, + JsonRpcNotification, + isJsonRpcRequest, + isNullOrUndefined, } from '@metamask/utils'; import { errorCodes, EthereumRpcError, serializeError } from 'eth-rpc-errors'; @@ -36,6 +39,10 @@ export type JsonRpcMiddleware = ( end: JsonRpcEngineEndCallback, ) => void; +export type JsonRpcNotificationHandler = ( + notification: JsonRpcNotification, +) => void | Promise; + /** * A JSON-RPC request and response processor. * Give it a stack of middleware, pass it requests, and get back responses. @@ -43,9 +50,12 @@ export type JsonRpcMiddleware = ( export class JsonRpcEngine extends SafeEventEmitter { private _middleware: JsonRpcMiddleware[]; - constructor() { + private readonly _notificationHandler?: JsonRpcNotificationHandler; + + constructor(notificationHandler?: JsonRpcNotificationHandler) { super(); this._middleware = []; + this._notificationHandler = notificationHandler; } /** @@ -69,14 +79,26 @@ export class JsonRpcEngine extends SafeEventEmitter { ): void; /** - * Handle an array of JSON-RPC requests, and return an array of responses. + * Handle a JSON-RPC notification. + * + * @param notification - The notification to handle. + * @param callback - An error-first callback that will receive a `void` response. + */ + handle( + notification: JsonRpcNotification, + callback: (error: unknown, response: void) => void, + ): void; + + /** + * Handle an array of JSON-RPC requests and/or notifications, and return an + * array of responses to any included requests. * * @param request - The requests to handle. * @param callback - An error-first callback that will receive the array of * responses. */ handle( - requests: JsonRpcRequest[], + requests: (JsonRpcRequest | JsonRpcNotification)[], callback: (error: unknown, responses: JsonRpcResponse[]) => void, ): void; @@ -91,13 +113,21 @@ export class JsonRpcEngine extends SafeEventEmitter { ): Promise>; /** - * Handle an array of JSON-RPC requests, and return an array of responses. + * Handle a JSON-RPC notification. + * + * @param notification - The notification to handle. + */ + handle(notification: JsonRpcNotification): Promise; + + /** + * Handle an array of JSON-RPC requests and/or notifications, and return an + * array of responses to any included requests. * * @param request - The JSON-RPC requests to handle. * @returns An array of JSON-RPC responses. */ handle( - requests: JsonRpcRequest[], + requests: (JsonRpcRequest | JsonRpcNotification)[], ): Promise[]>; handle(req: unknown, callback?: any) { @@ -153,14 +183,14 @@ export class JsonRpcEngine extends SafeEventEmitter { * Like _handle, but for batch requests. */ private _handleBatch( - reqs: JsonRpcRequest[], + reqs: (JsonRpcRequest | JsonRpcNotification)[], ): Promise[]>; /** * Like _handle, but for batch requests. */ private _handleBatch( - reqs: JsonRpcRequest[], + reqs: (JsonRpcRequest | JsonRpcNotification)[], callback: (error: unknown, responses?: JsonRpcResponse[]) => void, ): Promise; @@ -173,17 +203,22 @@ export class JsonRpcEngine extends SafeEventEmitter { * @returns The array of responses, or nothing if a callback was specified. */ private async _handleBatch( - reqs: JsonRpcRequest[], + reqs: (JsonRpcRequest | JsonRpcNotification)[], callback?: (error: unknown, responses?: JsonRpcResponse[]) => void, ): Promise[] | void> { // The order here is important try { // 2. Wait for all requests to finish, or throw on some kind of fatal // error - const responses = await Promise.all( - // 1. Begin executing each request in the order received - reqs.map(this._promiseHandle.bind(this)), - ); + const responses = ( + await Promise.all( + // 1. Begin executing each request in the order received + reqs.map(this._promiseHandle.bind(this)), + // Filter out falsy responses from notifications + ) + ).filter( + (response) => !isNullOrUndefined(response), + ) as JsonRpcResponse[]; // 3. Return batch response if (callback) { @@ -206,8 +241,8 @@ export class JsonRpcEngine extends SafeEventEmitter { * @returns The JSON-RPC response. */ private _promiseHandle( - req: JsonRpcRequest, - ): Promise> { + req: JsonRpcRequest | JsonRpcNotification, + ): Promise | void> { return new Promise((resolve) => { this._handle(req, (_err, res) => { // There will always be a response, and it will always have any error @@ -218,8 +253,8 @@ export class JsonRpcEngine extends SafeEventEmitter { } /** - * Ensures that the request object is valid, processes it, and passes any - * error and the response object to the given callback. + * Ensures that the request / notification object is valid, processes it, and + * passes any error and response object to the given callback. * * Does not reject. * @@ -228,8 +263,8 @@ export class JsonRpcEngine extends SafeEventEmitter { * @returns Nothing. */ private async _handle( - callerReq: JsonRpcRequest, - callback: (error: unknown, response: JsonRpcResponse) => void, + callerReq: JsonRpcRequest | JsonRpcNotification, + callback: (error?: unknown, response?: JsonRpcResponse) => void, ): Promise { if ( !callerReq || @@ -250,18 +285,37 @@ export class JsonRpcEngine extends SafeEventEmitter { `Must specify a string method. Received: ${typeof callerReq.method}`, { request: callerReq }, ); - return callback(error, { id: callerReq.id, jsonrpc: '2.0', error }); + + if (isJsonRpcRequest(callerReq)) { + return callback(error, { + id: callerReq.id ?? null, + jsonrpc: '2.0', + error, + }); + } + // Do not reply to notifications, even malformed ones. + return callback(null); } + // Handle notifications. + // We can't use isJsonRpcNotification here because that narrows callerReq to + // "never" after the if clause for unknown reasons. + if (!isJsonRpcRequest(callerReq)) { + await this._notificationHandler?.(callerReq); + return callback(null); + } + + let error: JsonRpcEngineCallbackError = null; + + // Handle requests. const req: JsonRpcRequest = { ...callerReq }; const res: PendingJsonRpcResponse = { id: req.id, jsonrpc: req.jsonrpc, }; - let error: JsonRpcEngineCallbackError = null; try { - await this._processRequest(req, res); + await JsonRpcEngine._processRequest(req, res, this._middleware); } catch (_error) { // A request handler error, a re-thrown middleware error, or something // unexpected. @@ -286,13 +340,15 @@ export class JsonRpcEngine extends SafeEventEmitter { * * @param req - The request object. * @param res - The response object. + * @param middlewares - The stack of middleware functions. */ - private async _processRequest( + private static async _processRequest( req: JsonRpcRequest, res: PendingJsonRpcResponse, + middlewares: JsonRpcMiddleware[], ): Promise { const [error, isComplete, returnHandlers] = - await JsonRpcEngine._runAllMiddleware(req, res, this._middleware); + await JsonRpcEngine._runAllMiddleware(req, res, middlewares); // Throw if "end" was not called, or if the response has neither a result // nor an error. @@ -314,7 +370,7 @@ export class JsonRpcEngine extends SafeEventEmitter { * * @param req - The request object. * @param res - The response object. - * @param middlewareStack - The stack of middleware functions to execute. + * @param middlewares - The stack of middleware functions to execute. * @returns An array of any error encountered during middleware execution, * a boolean indicating whether the request was completed, and an array of * middleware-defined return handlers. @@ -322,7 +378,7 @@ export class JsonRpcEngine extends SafeEventEmitter { private static async _runAllMiddleware( req: JsonRpcRequest, res: PendingJsonRpcResponse, - middlewareStack: JsonRpcMiddleware[], + middlewares: JsonRpcMiddleware[], ): Promise< [ unknown, // error @@ -335,7 +391,7 @@ export class JsonRpcEngine extends SafeEventEmitter { let isComplete = false; // Go down stack of middleware, call and collect optional returnHandlers - for (const middleware of middlewareStack) { + for (const middleware of middlewares) { [error, isComplete] = await JsonRpcEngine._runMiddleware( req, res, From 602c51512e9b46bae8e476a99845b10548817865 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 17 May 2022 15:14:56 -0700 Subject: [PATCH 2/7] Complete test coverage --- src/JsonRpcEngine.test.ts | 59 ++++++++++++++++++++++++++++++++++----- src/JsonRpcEngine.ts | 6 +++- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/JsonRpcEngine.test.ts b/src/JsonRpcEngine.test.ts index 57731ea..a1e6bab 100644 --- a/src/JsonRpcEngine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -11,13 +11,13 @@ const jsonrpc = '2.0' as const; describe('JsonRpcEngine', () => { it('handle: throws on truthy, non-function callback', () => { - const engine: any = new JsonRpcEngine(); - expect(() => engine.handle({}, true)).toThrow( + const engine = new JsonRpcEngine(); + expect(() => engine.handle({} as any, 'foo' as any)).toThrow( '"callback" must be a function if provided.', ); }); - it('handle: returns error for invalid request parameter', async () => { + it('handle: returns error for invalid request', async () => { const engine = new JsonRpcEngine(); let response: any = await engine.handle(null as any); expect(response.error.code).toStrictEqual(-32600); @@ -30,13 +30,58 @@ describe('JsonRpcEngine', () => { it('handle: returns error for invalid request method', async () => { const engine = new JsonRpcEngine(); - let response: any = await engine.handle({ id: 1, method: null } as any); + const response: any = await engine.handle({ id: 1, method: null } as any); + expect(response.error.code).toStrictEqual(-32600); expect(response.result).toBeUndefined(); + }); + + it('handle: returns error for invalid request method with nullish id', async () => { + const engine = new JsonRpcEngine(); + const response: any = await engine.handle({ + id: undefined, + method: null, + } as any); + + expect(response.error.code).toStrictEqual(-32600); + expect(response.result).toBeUndefined(); + }); - // No response if duck-typed as a notification - response = await engine.handle({ method: true } as any); - expect(response).toBeUndefined(); + it('handle: returns undefined for malformed notifications', async () => { + const middleware = jest.fn(); + const notificationHandler = jest.fn(); + const engine = new JsonRpcEngine({ notificationHandler }); + engine.push(middleware); + + expect( + await engine.handle({ jsonrpc, method: true } as any), + ).toBeUndefined(); + expect(notificationHandler).not.toHaveBeenCalled(); + expect(middleware).not.toHaveBeenCalled(); + }); + + it('handle: ignores handlers when no notification is specified', async () => { + const middleware = jest.fn(); + const engine = new JsonRpcEngine(); + engine.push(middleware); + + expect(await engine.handle({ jsonrpc, method: 'foo' })).toBeUndefined(); + expect(middleware).not.toHaveBeenCalled(); + }); + + it('handle: forwards notifications to handlers', async () => { + const middleware = jest.fn(); + const notificationHandler = jest.fn(); + const engine = new JsonRpcEngine({ notificationHandler }); + engine.push(middleware); + + expect(await engine.handle({ jsonrpc, method: 'foo' })).toBeUndefined(); + expect(notificationHandler).toHaveBeenCalledTimes(1); + expect(notificationHandler).toHaveBeenCalledWith({ + jsonrpc, + method: 'foo', + }); + expect(middleware).not.toHaveBeenCalled(); }); it('handle: basic middleware test 1', async () => { diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 843ec88..785ce33 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -43,6 +43,10 @@ export type JsonRpcNotificationHandler = ( notification: JsonRpcNotification, ) => void | Promise; +interface JsonRpcEngineArgs { + notificationHandler?: JsonRpcNotificationHandler; +} + /** * A JSON-RPC request and response processor. * Give it a stack of middleware, pass it requests, and get back responses. @@ -52,7 +56,7 @@ export class JsonRpcEngine extends SafeEventEmitter { private readonly _notificationHandler?: JsonRpcNotificationHandler; - constructor(notificationHandler?: JsonRpcNotificationHandler) { + constructor({ notificationHandler }: JsonRpcEngineArgs = {}) { super(); this._middleware = []; this._notificationHandler = notificationHandler; From a57f8295842d95ec833296a6f774702e6b69d1c6 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 17 May 2022 17:03:44 -0700 Subject: [PATCH 3/7] Only handle notifications when a handler is specified --- src/JsonRpcEngine.test.ts | 15 +++++++++++---- src/JsonRpcEngine.ts | 35 ++++++++++++++++++++++++----------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/JsonRpcEngine.test.ts b/src/JsonRpcEngine.test.ts index a1e6bab..2f731c8 100644 --- a/src/JsonRpcEngine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -60,13 +60,20 @@ describe('JsonRpcEngine', () => { expect(middleware).not.toHaveBeenCalled(); }); - it('handle: ignores handlers when no notification is specified', async () => { - const middleware = jest.fn(); + it('handle: treats notifications as requests when no notification handler is specified', async () => { + const middleware = jest.fn().mockImplementation((_req, res, _next, end) => { + res.result = 'bar'; + end(); + }); const engine = new JsonRpcEngine(); engine.push(middleware); - expect(await engine.handle({ jsonrpc, method: 'foo' })).toBeUndefined(); - expect(middleware).not.toHaveBeenCalled(); + expect(await engine.handle({ jsonrpc, method: 'foo' })).toStrictEqual({ + jsonrpc, + result: 'bar', + id: undefined, + }); + expect(middleware).toHaveBeenCalledTimes(1); }); it('handle: forwards notifications to handlers', async () => { diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 785ce33..3b8dcc3 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -56,6 +56,17 @@ export class JsonRpcEngine extends SafeEventEmitter { private readonly _notificationHandler?: JsonRpcNotificationHandler; + /** + * Constructs a {@link JsonRpcEngine} instance. + * + * @param options - Options bag. + * @param options.notificationHandler - A function for handling JSON-RPC + * notifications. A JSON-RPC notification is defined as a JSON-RPC request + * without an `id` property. If this option is not provided, notifications + * will be treated the same as requests. If this option _is_ provided, + * notifications will be passed to the handler function without touching + * the engine's middleware stack. + */ constructor({ notificationHandler }: JsonRpcEngineArgs = {}) { super(); this._middleware = []; @@ -290,29 +301,31 @@ export class JsonRpcEngine extends SafeEventEmitter { { request: callerReq }, ); - if (isJsonRpcRequest(callerReq)) { - return callback(error, { - id: callerReq.id ?? null, - jsonrpc: '2.0', - error, - }); + if (this._notificationHandler && !isJsonRpcRequest(callerReq)) { + // Do not reply to notifications, even malformed ones. + return callback(null); } - // Do not reply to notifications, even malformed ones. - return callback(null); + + return callback(error, { + id: (callerReq as JsonRpcRequest).id ?? null, + 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 (!isJsonRpcRequest(callerReq)) { - await this._notificationHandler?.(callerReq); + if (this._notificationHandler && !isJsonRpcRequest(callerReq)) { + await this._notificationHandler(callerReq); return callback(null); } let error: JsonRpcEngineCallbackError = null; // Handle requests. - const req: JsonRpcRequest = { ...callerReq }; + // Typecast: Permit missing id's for backwards compatibility. + const req = { ...(callerReq as JsonRpcRequest) }; const res: PendingJsonRpcResponse = { id: req.id, jsonrpc: req.jsonrpc, From 0032493bdfa959d7e71c0ae8460ffb77efe99dd3 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 17 May 2022 17:35:48 -0700 Subject: [PATCH 4/7] Improve documentation --- README.md | 16 ++++++++++++++-- src/JsonRpcEngine.ts | 9 ++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ab0b9d2..6114f56 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A tool for processing JSON-RPC requests and responses. ```js const { JsonRpcEngine } = require('json-rpc-engine'); -let engine = new JsonRpcEngine(); +const engine = new JsonRpcEngine(); ``` Build a stack of JSON-RPC processors by pushing middleware to the engine. @@ -22,7 +22,7 @@ engine.push(function (req, res, next, end) { Requests are handled asynchronously, stepping down the stack until complete. ```js -let request = { id: 1, jsonrpc: '2.0', method: 'hello' }; +const request = { id: 1, jsonrpc: '2.0', method: 'hello' }; engine.handle(request, function (err, response) { // Do something with response.result, or handle response.error @@ -53,6 +53,18 @@ engine.push(function (req, res, next, end) { }); ``` +If you specify a `notificationHandler` when constructing the engine, JSON-RPC notifications passed to `handle()` will be handed off directly to this function without touching the middleware stack: + +```js +const engine = new JsonRpcEngine({ notificationHandler }); + +// A notification is defined as a JSON-RPC request without an `id` property. +const notification = { jsonrpc: '2.0', method: 'hello' }; + +const response = await engine.handle(notification); +console.log(typeof response); // 'undefined' +``` + Engines can be nested by converting them to middleware using `JsonRpcEngine.asMiddleware()`: ```js diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 3b8dcc3..2ea8436 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -44,6 +44,13 @@ export type JsonRpcNotificationHandler = ( ) => void | Promise; interface JsonRpcEngineArgs { + /** + * A function for handling JSON-RPC notifications. A JSON-RPC notification is + * defined as a JSON-RPC request without an `id` property. If this option is + * _not_ provided, notifications will be treated the same as requests. If this + * option _is_ provided, notifications will be passed to the handler + * function without touching the engine's middleware stack. + */ notificationHandler?: JsonRpcNotificationHandler; } @@ -62,7 +69,7 @@ export class JsonRpcEngine extends SafeEventEmitter { * @param options - Options bag. * @param options.notificationHandler - A function for handling JSON-RPC * notifications. A JSON-RPC notification is defined as a JSON-RPC request - * without an `id` property. If this option is not provided, notifications + * without an `id` property. If this option is _not_ provided, notifications * will be treated the same as requests. If this option _is_ provided, * notifications will be passed to the handler function without touching * the engine's middleware stack. From 907a6da0264da9e7067e10980477ee2960bf5ce6 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 17 May 2022 22:08:35 -0700 Subject: [PATCH 5/7] Fix notification handler error logic --- src/JsonRpcEngine.test.ts | 37 +++++++++++++++++++++++++++++++++++++ src/JsonRpcEngine.ts | 30 +++++++++++++++++++++--------- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/JsonRpcEngine.test.ts b/src/JsonRpcEngine.test.ts index 2f731c8..16f0180 100644 --- a/src/JsonRpcEngine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -91,6 +91,43 @@ describe('JsonRpcEngine', () => { expect(middleware).not.toHaveBeenCalled(); }); + it('handle: re-throws errors from notification handlers (async)', async () => { + const notificationHandler = jest.fn().mockImplementation(() => { + throw new Error('baz'); + }); + const engine = new JsonRpcEngine({ notificationHandler }); + + await expect(engine.handle({ jsonrpc, method: 'foo' })).rejects.toThrow( + new Error('baz'), + ); + expect(notificationHandler).toHaveBeenCalledTimes(1); + expect(notificationHandler).toHaveBeenCalledWith({ + jsonrpc, + method: 'foo', + }); + }); + + it('handle: re-throws errors from notification handlers (callback)', async () => { + const notificationHandler = jest.fn().mockImplementation(() => { + throw new Error('baz'); + }); + const engine = new JsonRpcEngine({ notificationHandler }); + + await new Promise((resolve) => { + engine.handle({ jsonrpc, method: 'foo' }, (error, response) => { + expect(error).toStrictEqual(new Error('baz')); + expect(response).toBeUndefined(); + + expect(notificationHandler).toHaveBeenCalledTimes(1); + expect(notificationHandler).toHaveBeenCalledWith({ + jsonrpc, + method: 'foo', + }); + resolve(); + }); + }); + }); + it('handle: basic middleware test 1', async () => { const engine = new JsonRpcEngine(); diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 2ea8436..81ca8fb 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -6,7 +6,6 @@ import { JsonRpcResponse, JsonRpcNotification, isJsonRpcRequest, - isNullOrUndefined, } from '@metamask/utils'; import { errorCodes, EthereumRpcError, serializeError } from 'eth-rpc-errors'; @@ -49,7 +48,8 @@ interface JsonRpcEngineArgs { * defined as a JSON-RPC request without an `id` property. If this option is * _not_ provided, notifications will be treated the same as requests. If this * option _is_ provided, notifications will be passed to the handler - * function without touching the engine's middleware stack. + * function without touching the engine's middleware stack. This function + * should not** throw or reject. */ notificationHandler?: JsonRpcNotificationHandler; } @@ -72,7 +72,8 @@ export class JsonRpcEngine extends SafeEventEmitter { * without an `id` property. If this option is _not_ provided, notifications * will be treated the same as requests. If this option _is_ provided, * notifications will be passed to the handler function without touching - * the engine's middleware stack. + * the engine's middleware stack. This function **should not** throw or + * reject. */ constructor({ notificationHandler }: JsonRpcEngineArgs = {}) { super(); @@ -239,7 +240,8 @@ export class JsonRpcEngine extends SafeEventEmitter { // Filter out falsy responses from notifications ) ).filter( - (response) => !isNullOrUndefined(response), + // Filter out any notification responses. + (response) => response !== undefined, ) as JsonRpcResponse[]; // 3. Return batch response @@ -265,10 +267,16 @@ export class JsonRpcEngine extends SafeEventEmitter { private _promiseHandle( req: JsonRpcRequest | JsonRpcNotification, ): Promise | void> { - return new Promise((resolve) => { - this._handle(req, (_err, res) => { - // There will always be a response, and it will always have any error - // that is caught and propagated. + return new Promise((resolve, reject) => { + this._handle(req, (error, res) => { + // For notifications, the response will be `undefined`, and any caught + // errors are unexpected and should be surfaced to the caller. + if (error && res === undefined) { + reject(error); + } + + // Excepting notifications, there will always be a response, and it will + // always have any error that is caught and propagated. resolve(res); }); }); @@ -324,7 +332,11 @@ export class JsonRpcEngine extends SafeEventEmitter { // We can't use isJsonRpcNotification here because that narrows callerReq to // "never" after the if clause for unknown reasons. if (this._notificationHandler && !isJsonRpcRequest(callerReq)) { - await this._notificationHandler(callerReq); + try { + await this._notificationHandler(callerReq); + } catch (error) { + return callback(error); + } return callback(null); } From bf99a2f99278a8e29e5536a12d192618a955f009 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 18 May 2022 15:59:03 -0700 Subject: [PATCH 6/7] fixup! Merge branch 'main' into handle-notifications --- src/JsonRpcEngine.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/JsonRpcEngine.test.ts b/src/JsonRpcEngine.test.ts index 92f2a13..d807ae1 100644 --- a/src/JsonRpcEngine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -17,7 +17,6 @@ describe('JsonRpcEngine', () => { ); }); - it('handle: returns error for invalid request value', async () => { const engine = new JsonRpcEngine(); let response: any = await engine.handle(null as any); From 70327b1fe0368f7045689c67ade4cfaa66120216 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Fri, 12 Aug 2022 00:04:53 -0700 Subject: [PATCH 7/7] Respond to review --- src/JsonRpcEngine.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index b22cdd4..e101271 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -54,8 +54,9 @@ interface JsonRpcEngineArgs { * defined as a JSON-RPC request without an `id` property. If this option is * _not_ provided, notifications will be treated the same as requests. If this * option _is_ provided, notifications will be passed to the handler - * function without touching the engine's middleware stack. This function - * should not** throw or reject. + * function without touching the engine's middleware stack. + * + * This function should not throw or reject. */ notificationHandler?: JsonRpcNotificationHandler; } @@ -83,8 +84,7 @@ export class JsonRpcEngine extends SafeEventEmitter { * without an `id` property. If this option is _not_ provided, notifications * will be treated the same as requests. If this option _is_ provided, * notifications will be passed to the handler function without touching - * the engine's middleware stack. This function **should not** throw or - * reject. + * the engine's middleware stack. This function should not throw or reject. */ constructor({ notificationHandler }: JsonRpcEngineArgs = {}) { super(); @@ -283,7 +283,6 @@ export class JsonRpcEngine extends SafeEventEmitter { await Promise.all( // 1. Begin executing each request in the order received reqs.map(this._promiseHandle.bind(this)), - // Filter out falsy responses from notifications ) ).filter( // Filter out any notification responses. @@ -363,11 +362,13 @@ export class JsonRpcEngine extends SafeEventEmitter { ); if (this._notificationHandler && !isJsonRpcRequest(callerReq)) { - // Do not reply to notifications, even malformed ones. + // Do not reply to notifications, even if they are malformed. return callback(null); } 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, jsonrpc: '2.0', error,