RFC: Remove callback from next return handlers

README.md

@@ -39,107 +39,78 @@ 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(function(req, res, next, end){
-  next(function(cb){
-    insertIntoCache(res, cb)
-  })
+engine.push(async function(req, res, next, end){
+  if (req.method !== targetMethod) return next()
+  res.result = await processTargetMethodRequest(req)
+  return end()
 })
 ```
 
-Engines can be nested by converting them to middleware using `JsonRpcEngine.asMiddleware()`:
+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
-const engine = new JsonRpcEngine()
-const subengine = new JsonRpcEngine()
-engine.push(subengine.asMiddleware())
+engine.push((req, res, next, end) => {
+  next(async () => {
+    await insertIntoCache(res)
+  })
+})
 ```
 
-### `async` Middleware
+Return handlers can be synchronous or asynchronous.
+They take no callbacks, and should only interact with the request and/or the response.
 
-If you require your middleware function to be `async`, use `createAsyncMiddleware`:
+Engines can be nested by converting them to middleware using `JsonRpcEngine.asMiddleware()`:
 
 ```js
-const { createAsyncMiddleware } = require('json-rpc-engine')
-
-let engine = new RpcEngine()
-engine.push(createAsyncMiddleware(async (req, res, next) => {
-  res.result = 42
-  next()
-}))
+const engine = new JsonRpcEngine()
+const subengine = new JsonRpcEngine()
+engine.push(subengine.asMiddleware())
 ```
 
-`async` middleware do not take an `end` callback.
-Instead, the request ends if the middleware returns without calling `next()`:
+### Error Handling
 
-```js
-engine.push(createAsyncMiddleware(async (req, res, next) => {
-  res.result = 42
-  /* The request will end when this returns */
-}))
-```
+Errors should be handled by throwing inside middleware functions.
 
-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.
+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
-  await next()
-  /* Your return handler logic goes here */
-  addToMetrics(res)
-}))
-```
+Errors always take precedent over results.
+If an error is detected, the response's `result` property will be deleted.
 
-You can freely mix callback-based and `async` middleware:
+All of the following examples are equivalent.
+It does not matter of the middleware function is synchronous or asynchronous.
 
 ```js
+// Throwing is preferred.
 engine.push(function(req, res, next, end){
-  if (!isCached(req)) {
-    return next((cb) => {
-      insertIntoCache(res, cb)
-    })
-  }
-  res.result = getResultFromCache(req)
-  end()
+  throw new Error()
 })
 
-engine.push(createAsyncMiddleware(async (req, res, next) => {
-  res.result = 42
-  await next()
-  addToMetrics(res)
-}))
-```
-
-### Gotchas
-
-Handle errors via `end(err)`, *NOT* `next(err)`.
-
-```js
-/* INCORRECT */
+// For backwards compatibility, you can also do this:
 engine.push(function(req, res, next, end){
-  next(new Error())
+  end(new Error())
 })
 
-/* CORRECT */
 engine.push(function(req, res, next, end){
-  end(new Error())
+  res.error = new Error()
+  end()
 })
-```
 
-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. */
+  next()
+})
+
+// INCORRECT. Do not do this:
+engine.push(function(req, res, next, end){
+  next(new Error())
 })
 ```

package.json

@@ -14,8 +14,9 @@
   "scripts": {
     "build": "tsc --project .",
     "lint": "eslint . --ext ts,js,json",
-    "lint:fix": "eslint . --ext ts,js,json --fix",
-    "test": "mocha ./test",
+    "lint:fix": "yarn lint --fix",
+    "test:nobuild": "mocha ./test",
+    "test": "yarn build && yarn test:nobuild",
     "coverage": "nyc --check-coverage yarn test",
     "prepublishOnly": "yarn && yarn lint && yarn build && yarn coverage"
   },

src/JsonRpcEngine.ts

@@ -67,9 +67,7 @@ export interface PendingJsonRpcResponse<T> extends JsonRpcResponseBase {
 
 export type JsonRpcEngineCallbackError = Error | JsonRpcError | null;
 
-export type JsonRpcEngineReturnHandler = (
-  done: (error?: JsonRpcEngineCallbackError) => void
-) => void;
+export type JsonRpcEngineReturnHandler = () => void | Promise<void>;
 
 export type JsonRpcEngineNextCallback = (
   returnHandlerCallback?: JsonRpcEngineReturnHandler
@@ -84,7 +82,7 @@ export type JsonRpcMiddleware<T, U> = (
   res: PendingJsonRpcResponse<U>,
   next: JsonRpcEngineNextCallback,
   end: JsonRpcEngineEndCallback
-) => void;
+) => void | Promise<void>;
 
 /**
  * A JSON-RPC request and response processor.
@@ -186,13 +184,8 @@ export class JsonRpcEngine extends SafeEventEmitter {
           return end(middlewareError as JsonRpcEngineCallbackError);
         }
 
-        return next(async (handlerCallback) => {
-          try {
-            await JsonRpcEngine._runReturnHandlers(returnHandlers);
-          } catch (error) {
-            return handlerCallback(error);
-          }
-          return handlerCallback();
+        return next(async () => {
+          await JsonRpcEngine._runReturnHandlers(returnHandlers);
         });
       } catch (error) {
         return end(error);
@@ -388,55 +381,61 @@ export class JsonRpcEngine extends SafeEventEmitter {
    * @returns An array of any error encountered during middleware exection,
    * and a boolean indicating whether the request should end.
    */
-  private static _runMiddleware(
+  private static async _runMiddleware(
     req: JsonRpcRequest<unknown>,
     res: PendingJsonRpcResponse<unknown>,
     middleware: JsonRpcMiddleware<unknown, unknown>,
     returnHandlers: JsonRpcEngineReturnHandler[],
   ): Promise<[unknown, boolean]> {
-    return new Promise((resolve) => {
-      const end: JsonRpcEngineEndCallback = (err?: unknown) => {
-        const error = err || res.error;
-        if (error) {
-          res.error = serializeError(error);
-        }
-        // True indicates that the request should end
-        resolve([error, true]);
-      };
-
-      const next: JsonRpcEngineNextCallback = (
-        returnHandler?: JsonRpcEngineReturnHandler,
-      ) => {
-        if (res.error) {
-          end(res.error);
-        } else {
-          if (returnHandler) {
-            if (typeof returnHandler !== 'function') {
-              end(
-                new EthereumRpcError(
-                  errorCodes.rpc.internal,
-                  `JsonRpcEngine: "next" return handlers must be functions. ` +
-                    `Received "${typeof returnHandler}" for request:\n${jsonify(
-                      req,
-                    )}`,
-                  { request: req },
-                ),
-              );
-            }
-            returnHandlers.push(returnHandler);
-          }
+    let resolve: (value: [unknown, boolean]) => void;
+    const middlewareCallbackPromise = new Promise<[unknown, boolean]>(
+      (_resolve) => {
+        resolve = _resolve;
+      },
+    );
+
+    const end: JsonRpcEngineEndCallback = (err?: unknown) => {
+      const error = err || res.error;
+      if (error) {
+        res.error = serializeError(error);
+      }
+      // True indicates that the request should end
+      resolve([error, true]);
+    };
 
-          // False indicates that the request should not end
-          resolve([null, false]);
+    const next: JsonRpcEngineNextCallback = (
+      returnHandler?: JsonRpcEngineReturnHandler,
+    ) => {
+      if (res.error) {
+        end(res.error);
+      } else {
+        if (returnHandler) {
+          if (typeof returnHandler !== 'function') {
+            end(
+              new EthereumRpcError(
+                errorCodes.rpc.internal,
+                `JsonRpcEngine: "next" return handlers must be functions. ` +
+                  `Received "${typeof returnHandler}" for request:\n${jsonify(
+                    req,
+                  )}`,
+                { request: req },
+              ),
+            );
+          }
+          returnHandlers.push(returnHandler);
         }
-      };
 
-      try {
-        middleware(req, res, next, end);
-      } catch (error) {
-        end(error);
+        // False indicates that the request should not end
+        resolve([null, false]);
       }
-    });
+    };
+
+    try {
+      await middleware(req, res, next, end);
+    } catch (error) {
+      end(error);
+    }
+    return middlewareCallbackPromise;
   }
 
   /**
@@ -447,9 +446,7 @@ export class JsonRpcEngine extends SafeEventEmitter {
     handlers: JsonRpcEngineReturnHandler[],
   ): Promise<void> {
     for (const handler of handlers) {
-      await new Promise<void>((resolve, reject) => {
-        handler((err) => (err ? reject(err) : resolve()));
-      });
+      await handler();
     }
   }