RFC: Remove next callbacks

README.md

@@ -5,82 +5,86 @@ A tool for processing JSON-RPC requests and responses.
 ## Usage
 
 ```js
-const { JsonRpcEngine } = require('json-rpc-engine')
+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.
 
 ```js
-engine.push(function(req, res, next, end){
-  res.result = 42
-  end()
-})
+engine.push(function (req, res, end) {
+  res.result = 42;
+  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){
+engine.handle(request, (err, response) => {
   // Do something with response.result, or handle response.error
-})
+});
 
 // There is also a Promise signature
-const response = await engine.handle(request)
+const response = await engine.handle(request);
 ```
 
 Middleware have direct access to the request and response objects.
-They can let processing continue down the stack with `next()`, or complete the request with `end()`.
+They can let processing continue down the stack by returning, or complete the request with `end()`.
 
 ```js
-engine.push(function(req, res, next, end){
-  if (req.skipCache) return next()
-  res.result = getResultFromCache(req)
-  return end()
-})
+engine.push(function (req, res, end) {
+  if (req.skipCache) return;
+  res.result = getResultFromCache(req);
+  return end();
+});
 ```
 
 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()
-})
+engine.push(async function (req, res, end) {
+  if (req.method !== targetMethod) return;
+  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.
+By returning a _return handler function_, middleware can interact with the response before it is returned to the requester.
 
 ```js
-engine.push((req, res, next, end) => {
-  next(() => {
-    await insertIntoCache(res)
-  })
+engine.push((req, res, end) => {
+  return async () => {
+    await insertIntoCache(res);
+  }
 })
 ```
 
 Return handlers can be synchronous or asynchronous.
 They take no callbacks, and should only interact with the request and/or the response.
 
+Middleware functions **must** return a falsy value or a function.
+If anything else is returned, the request will end with an error.
+
+If a middleware calls `end()`, its return value will be ignored.
+
 Engines can be nested by converting them to middleware using `JsonRpcEngine.asMiddleware()`:
 
 ```js
-const engine = new JsonRpcEngine()
-const subengine = new JsonRpcEngine()
-engine.push(subengine.asMiddleware())
+const engine = new JsonRpcEngine();
+const subengine = new JsonRpcEngine();
+engine.push(subengine.asMiddleware());
 ```
 
 ### Error Handling
 
 Errors should be handled by throwing inside middleware functions.
 
 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.
+or set the error on the response object, and then return or call `end`.
 
 Errors always take precedent over results.
 If an error is detected, the response's `result` property will be deleted.
@@ -90,27 +94,21 @@ It does not matter of the middleware function is synchronous or asynchronous.
 
 ```js
 // Throwing is preferred.
-engine.push(function(req, res, next, end){
-  throw new Error()
-})
+engine.push(function (req, res, end) {
+  throw new Error();
+});
 
 // For backwards compatibility, you can also do this:
-engine.push(function(req, res, next, end){
-  end(new Error())
-})
-
-engine.push(function(req, res, next, end){
-  res.error = new Error()
-  end()
-})
-
-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())
-})
+engine.push(function (req, res, end) {
+  end(new Error());
+});
+
+engine.push(function (req, res, end) {
+  res.error = new Error();
+  end();
+});
+
+engine.push(function (req, res, end) {
+  res.error = new Error();
+});
 ```

src/JsonRpcEngine.ts

@@ -67,11 +67,9 @@ export interface PendingJsonRpcResponse<T> extends JsonRpcResponseBase {
 
 export type JsonRpcEngineCallbackError = Error | JsonRpcError | null;
 
-export type JsonRpcEngineReturnHandler = () => void | Promise<void>;
+type MaybePromise<T> = Promise<T> | T;
 
-export type JsonRpcEngineNextCallback = (
-  returnHandlerCallback?: JsonRpcEngineReturnHandler
-) => void;
+export type JsonRpcEngineReturnHandler = () => MaybePromise<void>;
 
 export type JsonRpcEngineEndCallback = (
   error?: JsonRpcEngineCallbackError
@@ -80,9 +78,8 @@ export type JsonRpcEngineEndCallback = (
 export type JsonRpcMiddleware<T, U> = (
   req: JsonRpcRequest<T>,
   res: PendingJsonRpcResponse<U>,
-  next: JsonRpcEngineNextCallback,
   end: JsonRpcEngineEndCallback
-) => void | Promise<void>;
+) => MaybePromise<void | JsonRpcEngineReturnHandler>;
 
 /**
  * A JSON-RPC request and response processor.
@@ -171,7 +168,7 @@ export class JsonRpcEngine extends SafeEventEmitter {
    * @returns This engine as a middleware function.
    */
   asMiddleware(): JsonRpcMiddleware<unknown, unknown> {
-    return async (req, res, next, end) => {
+    return async (req, res, end) => {
       try {
         const [
           middlewareError,
@@ -184,9 +181,9 @@ export class JsonRpcEngine extends SafeEventEmitter {
           return end(middlewareError as JsonRpcEngineCallbackError);
         }
 
-        return next(async () => {
+        return async () => {
           await JsonRpcEngine._runReturnHandlers(returnHandlers);
-        });
+        };
       } catch (error) {
         return end(error);
       }
@@ -387,51 +384,51 @@ export class JsonRpcEngine extends SafeEventEmitter {
     middleware: JsonRpcMiddleware<unknown, unknown>,
     returnHandlers: JsonRpcEngineReturnHandler[],
   ): Promise<[unknown, boolean]> {
-    let resolve: (value: [unknown, boolean]) => void;
-    const middlewareCallbackPromise = new Promise<[unknown, boolean]>(
-      (_resolve) => {
-        resolve = _resolve;
-      },
-    );
+    const [
+      middlewareCallbackPromise,
+      resolve,
+    ] = getDeferredPromise<[unknown, boolean]>();
 
+    let ended = false;
     const end: JsonRpcEngineEndCallback = (err?: unknown) => {
       const error = err || res.error;
       if (error) {
         res.error = serializeError(error);
       }
+
       // True indicates that the request should end
+      ended = true;
       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 },
-              ),
-            );
+    try {
+      const returnHandler = await middleware(req, res, end);
+
+      // If the request is already ended, there's nothing to do.
+      if (!ended) {
+        if (res.error) {
+          end(res.error);
+        } else {
+          if (returnHandler) {
+            if (typeof returnHandler !== 'function') {
+              end(
+                new EthereumRpcError(
+                  errorCodes.rpc.internal,
+                  `JsonRpcEngine: return handlers must be functions. ` +
+                    `Received "${typeof returnHandler}" for request:\n${jsonify(
+                      req,
+                    )}`,
+                  { request: req },
+                ),
+              );
+            }
+            returnHandlers.push(returnHandler);
           }
-          returnHandlers.push(returnHandler);
-        }
 
-        // False indicates that the request should not end
-        resolve([null, false]);
+          // False indicates that the request should not end
+          resolve([null, false]);
+        }
       }
-    };
-
-    try {
-      await middleware(req, res, next, end);
     } catch (error) {
       end(error);
     }
@@ -481,3 +478,11 @@ export class JsonRpcEngine extends SafeEventEmitter {
 function jsonify(request: JsonRpcRequest<unknown>): string {
   return JSON.stringify(request, null, 2);
 }
+
+function getDeferredPromise<T>(): [ Promise<T>, (value: T) => void] {
+  let resolve: any;
+  const promise: Promise<T> = new Promise((_resolve) => {
+    resolve = _resolve;
+  });
+  return [promise, resolve];
+}

src/createScaffoldMiddleware.ts

@@ -5,15 +5,15 @@ type ScaffoldMiddlewareHandler<T, U> = JsonRpcMiddleware<T, U> | Json;
 export function createScaffoldMiddleware(handlers: {
   [methodName: string]: ScaffoldMiddlewareHandler<unknown, unknown>;
 }): JsonRpcMiddleware<unknown, unknown> {
-  return (req, res, next, end) => {
+  return (req, res, end) => {
     const handler = handlers[req.method];
     // if no handler, return
     if (handler === undefined) {
-      return next();
+      return undefined;
     }
     // if handler is fn, call as middleware
     if (typeof handler === 'function') {
-      return handler(req, res, next, end);
+      return handler(req, res, end);
     }
     // if handler is some other value, use as result
     (res as JsonRpcSuccess<unknown>).result = handler;

src/idRemapMiddleware.ts

@@ -2,14 +2,14 @@ import { getUniqueId } from './getUniqueId';
 import { JsonRpcMiddleware } from './JsonRpcEngine';
 
 export function createIdRemapMiddleware(): JsonRpcMiddleware<unknown, unknown> {
-  return (req, res, next, _end) => {
+  return (req, res, _end) => {
     const originalId = req.id;
     const newId = getUniqueId();
     req.id = newId;
     res.id = newId;
-    next(() => {
+    return () => {
       req.id = originalId;
       res.id = originalId;
-    });
+    };
   };
 }