Allow middleware to be async, deprecate createAsyncMiddleware

README.md

@@ -39,11 +39,21 @@ They can let processing continue down the stack with `next()`, or complete the r
 engine.push(function (req, res, next, end) {
   if (req.skipCache) return next();
   res.result = getResultFromCache(req);
-  end();
+  return end();
 });
 ```
 
-By passing a _return handler_ to the `next` function, you can get a peek at the result before it returns.
+Middleware functions can be `async`:
+
+```js
+engine.push(async function (req, res, next, end) {
+  if (req.method !== targetMethod) return next();
+  res.result = await processTargetMethodRequest(req);
+  return end();
+});
+```
+
+By passing a _return handler_ to the `next` function, you can get a peek at the response before it is returned to the requester.
 
 ```js
 engine.push(function (req, res, next, end) {
@@ -73,69 +83,45 @@ const subengine = new JsonRpcEngine();
 engine.push(subengine.asMiddleware());
 ```
 
-### `async` Middleware
+### Error Handling
 
-If you require your middleware function to be `async`, use `createAsyncMiddleware`:
+Errors should be handled by throwing inside middleware functions.
 
-```js
-const { createAsyncMiddleware } = require('@metamask/json-rpc-engine');
-
-let engine = new RpcEngine();
-engine.push(
-  createAsyncMiddleware(async (req, res, next) => {
-    res.result = 42;
-    next();
-  }),
-);
-```
-
-`async` middleware do not take an `end` callback.
-Instead, the request ends if the middleware returns without calling `next()`:
+For backwards compatibility, you can also pass an error to the `end` callback,
+or set the error on the response object, and then call `end` or `next`.
+However, errors must **not** be passed to the `next` callback.
 
-```js
-engine.push(
-  createAsyncMiddleware(async (req, res, next) => {
-    res.result = 42;
-    /* The request will end when this returns */
-  }),
-);
-```
+Errors always take precedent over results.
+If an error is detected, the response's `result` property will be deleted.
 
-The `next` callback of `async` middleware also don't take return handlers.
-Instead, you can `await next()`.
-When the execution of the middleware resumes, you can work with the response again.
+All of the following examples are equivalent.
+It does not matter of the middleware function is synchronous or asynchronous.
 
 ```js
-engine.push(
-  createAsyncMiddleware(async (req, res, next) => {
-    res.result = 42;
-    await next();
-    /* Your return handler logic goes here */
-    addToMetrics(res);
-  }),
-);
-```
+// Throwing is preferred.
+engine.push(function (req, res, next, end) {
+  throw new Error();
+});
 
-You can freely mix callback-based and `async` middleware:
+// For backwards compatibility, you can also do this:
+engine.push(function (req, res, next, end) {
+  end(new Error());
+});
 
-```js
 engine.push(function (req, res, next, end) {
-  if (!isCached(req)) {
-    return next((cb) => {
-      insertIntoCache(res, cb);
-    });
-  }
-  res.result = getResultFromCache(req);
+  res.error = new Error();
   end();
 });
 
-engine.push(
-  createAsyncMiddleware(async (req, res, next) => {
-    res.result = 42;
-    await next();
-    addToMetrics(res);
-  }),
-);
+engine.push(function (req, res, next, end) {
+  res.error = new Error();
+  next();
+});
+
+// INCORRECT. Do not do this:
+engine.push(function (req, res, next, end) {
+  next(new Error());
+});
 ```
 
 ### Teardown
@@ -164,41 +150,3 @@ engine.destroy();
 // will throw an error.
 engine.handle(req);
 ```
-
-### Gotchas
-
-Handle errors via `end(err)`, _NOT_ `next(err)`.
-
-```js
-/* INCORRECT */
-engine.push(function (req, res, next, end) {
-  next(new Error());
-});
-
-/* CORRECT */
-engine.push(function (req, res, next, end) {
-  end(new Error());
-});
-```
-
-However, `next()` will detect errors on the response object, and cause
-`end(res.error)` to be called.
-
-```js
-engine.push(function (req, res, next, end) {
-  res.error = new Error();
-  next(); /* This will cause end(res.error) to be called. */
-});
-```
-
-## Running tests
-
-Build the project if not already built:
-
-```bash
-yarn build
-```
-
-```bash
-yarn test
-```

src/JsonRpcEngine.test.ts

@@ -534,8 +534,10 @@ describe('JsonRpcEngine', () => {
       });
     });
 
-    engine.push(function (_request, _response, next, _end) {
+    // Async middleware function
+    engine.push(async function (_request, _response, next, _end) {
       events.push('2-next');
+      await delay();
       next(function (callback) {
         events.push('2-return');
         callback();
@@ -590,6 +592,33 @@ describe('JsonRpcEngine', () => {
     });
   });
 
+  it('calls back next handler even if async middleware rejects', async () => {
+    const engine = new JsonRpcEngine();
+
+    let sawNextReturnHandlerCalled = false;
+
+    engine.push(function (_req, _res, next, _end) {
+      next(function (callback) {
+        sawNextReturnHandlerCalled = true;
+        callback();
+      });
+    });
+
+    engine.push(async function (_req, _res, _next, _end) {
+      throw new Error('boom');
+    });
+
+    const payload = { id: 1, jsonrpc, method: 'hello' };
+
+    await new Promise<void>((resolve) => {
+      engine.handle(payload, (error, _response) => {
+        expect(error).toBeDefined();
+        expect(sawNextReturnHandlerCalled).toBe(true);
+        resolve();
+      });
+    });
+  });
+
   it('handles error in next handler', async () => {
     const engine = new JsonRpcEngine();
 
@@ -708,3 +737,14 @@ describe('JsonRpcEngine', () => {
     });
   });
 });
+
+/**
+ * Delay for a number of milliseconds.
+ *
+ * @param ms - The number of milliseconds to delay.
+ */
+async function delay(ms = 1) {
+  return new Promise<void>((resolve) => {
+    setTimeout(() => resolve(), ms);
+  });
+}

src/JsonRpcEngine.ts

@@ -34,7 +34,7 @@ export type JsonRpcMiddleware<
     res: PendingJsonRpcResponse<Result>,
     next: JsonRpcEngineNextCallback,
     end: JsonRpcEngineEndCallback,
-  ): void;
+  ): void | Promise<void>;
   destroy?: () => void | Promise<void>;
 };
 
@@ -534,49 +534,55 @@ export class JsonRpcEngine extends SafeEventEmitter {
     middleware: JsonRpcMiddleware<JsonRpcParams, Json>,
     returnHandlers: JsonRpcEngineReturnHandler[],
   ): Promise<[unknown, boolean]> {
-    return new Promise((resolve) => {
-      const end: JsonRpcEngineEndCallback = (error?: unknown) => {
-        const parsedError = error || response.error;
-        if (parsedError) {
-          response.error = serializeError(parsedError);
-        }
-        // True indicates that the request should end
-        resolve([parsedError, true]);
-      };
-
-      const next: JsonRpcEngineNextCallback = (
-        returnHandler?: JsonRpcEngineReturnHandler,
-      ) => {
-        if (response.error) {
-          end(response.error);
-        } else {
-          if (returnHandler) {
-            if (typeof returnHandler !== 'function') {
-              end(
-                new JsonRpcError(
-                  errorCodes.rpc.internal,
-                  `JsonRpcEngine: "next" return handlers must be functions. ` +
-                    `Received "${typeof returnHandler}" for request:\n${jsonify(
-                      request,
-                    )}`,
-                  { request: request as Json },
-                ),
-              );
-            }
-            returnHandlers.push(returnHandler);
-          }
+    let resolve: (value: [unknown, boolean]) => void;
+    const middlewareCallbackPromise = new Promise<[unknown, boolean]>(
+      (_resolve) => {
+        resolve = _resolve;
+      },
+    );
 
-          // False indicates that the request should not end
-          resolve([null, false]);
-        }
-      };
+    const end: JsonRpcEngineEndCallback = (error?: unknown) => {
+      const parsedError = error || response.error;
+      if (parsedError) {
+        response.error = serializeError(parsedError);
+      }
+      // True indicates that the request should end
+      resolve([parsedError, true]);
+    };
 
-      try {
-        middleware(request, response, next, end);
-      } catch (error: any) {
-        end(error);
+    const next: JsonRpcEngineNextCallback = (
+      returnHandler?: JsonRpcEngineReturnHandler,
+    ) => {
+      if (response.error) {
+        return end(response.error);
       }
-    });
+
+      if (returnHandler) {
+        if (typeof returnHandler !== 'function') {
+          return end(
+            new JsonRpcError(
+              errorCodes.rpc.internal,
+              `JsonRpcEngine: "next" return handlers must be functions. ` +
+                `Received "${typeof returnHandler}" for request:\n${jsonify(
+                  request,
+                )}`,
+              { request: request as Json },
+            ),
+          );
+        }
+        returnHandlers.push(returnHandler);
+      }
+
+      // False indicates that the request should not end
+      return resolve([null, false]);
+    };
+
+    try {
+      await middleware(request, response, next, end);
+    } catch (error: any) {
+      end(error);
+    }
+    return middlewareCallbackPromise;
   }
 
   /**