Allow middleware to be async, deprecate createAsyncMiddleware #81

rekmarks · 2021-03-27T18:56:54Z

This PR lets any middleware function be async, and deprecates createAsyncMiddleware. The readme has been updated.

Previously, we did not handle rejections of plain, async middleware functions. createAsyncMiddleware was created in part to solve this problem. This PR introduces a simple refactor of #runMiddleware to handle rejections by async middleware functions.

It accomplishes this using a deferred promise, which was one of the techniques used to implement createAsyncMiddleware. Meanwhile, createAsyncMiddleware has been updated in order to retain backwards compatibility.

This change should be fully backwards-compatible.

Gudahtt

This generally seems promising! You really had me there with the title 😅 I thought you were getting rid of async middleware, but no, you're making all middleware optionally async.

I'd recommend leaving createAsyncMiddleware around in this PR and marking it as deprecated instead, to make migrating away from createAsyncMiddleware easier. We can remove it completely later on.

kumavis · 2021-04-12T01:18:04Z

I'd recommend leaving createAsyncMiddleware around in this PR and marking it as deprecated instead, to make migrating away from createAsyncMiddleware easier.

i think bumping major here is sufficient, others will still pull in the older version with the middleware util

kumavis · 2021-04-12T01:31:12Z

hmm, i actually dont like this.
the callback api and async api are different and shouldnt be mixed
better to add a engine.addAsyncMiddleware or just mandate that all middleware is async

kumavis · 2021-04-12T01:34:51Z

create async middleware currently allows you to await next() and does not have an end fn. the end is implied when the async middleware completes without calling next

rekmarks · 2021-05-17T20:41:46Z

the callback api and async api are different and shouldnt be mixed

@kumavis this is working up to resolving that problem. In #83 (dependency: #81 (this) -> #82 -> #83) I remove next callbacks completely in favor of a (req, res, end) signature where middleware can either be sync or async.

rekmarks · 2021-05-18T17:23:01Z

The main reason for removing next is that the async middleware pattern of await next(); /* do stuff */; return; invariably violates the node/callback-return ESLint rule. I agree with the authors of that rule that not returning the callback is an anti-pattern.

I decided to keep end in #83 because it seemed more explicit to call a function to end the processing of a request. That was purely an editorial decision, however, and we could instead decide to end the request whenever response.result or response.error are set, or when an error is thrown in a middleware.

* Handle errors fro masync middleware function rejections * Update readme * Update JsonRpcMiddleware type * Prevent promise resolution function from being called twice

We actually need this to please the linter. This reverts commit 36d91ad.

rekmarks · 2023-05-19T20:56:05Z

src/createScaffoldMiddleware.ts

@@ -19,7 +19,7 @@ type ScaffoldMiddlewareHandler<
 export function createScaffoldMiddleware(handlers: {
  [methodName: string]: ScaffoldMiddlewareHandler<JsonRpcParams, Json>;
 }): JsonRpcMiddleware<JsonRpcParams, Json> {
-  return (req, res, next, end) => {
+  return async (req, res, next, end) => {


The linter is asking for this, because middleware can now be async. We could create sync and async middleware types, not sure if it's worth it.

This seems fine. Make all the things async!

mcmire

Hi @rekmarks, I know you've been waiting for movement on this. I've had this on my list for a while and finally took the time to review this. I definitely learned some things while reviewing this, like how async middleware resolve the request via a callback passed to next, and that those callbacks are processed in reverse order when all middleware has been processed. I haven't 100% fully grokked how that works, but it does seem purely based on how it's used internally that next is less of a necessity and more of an implementation detail, so I'm glad that we're removing it to make things simpler.

Anyway, I really have only one suggestion here, but I understand your changes enough to say that they're good. I will monitor this PR this week so we can finally merge it.

mcmire · 2023-10-16T18:36:14Z

src/JsonRpcEngine.ts

+        returnHandlers.push(returnHandler);
+      }
+
+      // False indicates that the request should not end


Thought for another PR: I'm curious whether it would make more sense to make this an object rather than an array so we can make this self-documenting:

return resolve({ error: null, isComplete: false });

mcmire · 2023-10-16T18:36:45Z

src/JsonRpcEngine.ts

+
+    try {
+      await middleware(request, response, next, end);
+    } catch (error: any) {


Nit: Does it make more sense to use unknown rather than any here?

mcmire · 2023-10-16T18:38:10Z

src/createAsyncMiddleware.ts

 *
 * The return handler will always be called. Its resolution of the promise
 * enables the control flow described above.
 *
+ * @deprecated As of version 7.1.0, middleware functions can be asnync. This


Typo

Suggested change

* @deprecated As of version 7.1.0, middleware functions can be asnync. This

* @deprecated As of version 7.1.0, middleware functions can be async. This

mcmire · 2023-10-16T18:40:39Z

src/JsonRpcEngine.ts

-            }
-            returnHandlers.push(returnHandler);
-          }
+    let resolve: (value: [unknown, boolean]) => void;


Ah, I see, the reason why we have to do this now is because the function that the Promise constructor takes can't itself be async? Good solution.

mcmire · 2023-10-16T18:58:23Z

src/createAsyncMiddleware.ts

+        } else {
+          innerEnd(null);
+        }
+      } catch (error: any) {


Nit:

Suggested change

} catch (error: any) {

} catch (error: unknown) {

mcmire · 2023-10-16T18:59:37Z

src/createScaffoldMiddleware.ts

@@ -19,7 +19,7 @@ type ScaffoldMiddlewareHandler<
 export function createScaffoldMiddleware(handlers: {
  [methodName: string]: ScaffoldMiddlewareHandler<JsonRpcParams, Json>;
 }): JsonRpcMiddleware<JsonRpcParams, Json> {
-  return (req, res, next, end) => {
+  return async (req, res, next, end) => {


This seems fine. Make all the things async!

MajorLift · 2023-11-07T17:43:12Z

This library has now been migrated into the core monorepo. This PR has been locked and this repo will be archived shortly. Going forward, releases of this library will only include changes made in the core repo.

Please push this branch to core and open a new PR there.
Optionally, add a link pointing to the discussion in this PR to provide context.

rekmarks requested a review from a team as a code owner March 27, 2021 18:56

rekmarks mentioned this pull request Mar 28, 2021

RFC: Remove callback from next return handlers #82

Draft

Gudahtt reviewed Mar 30, 2021

View reviewed changes

rekmarks force-pushed the remove-createAsyncMiddleware branch from aca7998 to 1d9f07c Compare May 18, 2021 00:38

Remove createAsyncMiddleware

378a820

* Handle errors fro masync middleware function rejections * Update readme * Update JsonRpcMiddleware type * Prevent promise resolution function from being called twice

rekmarks force-pushed the remove-createAsyncMiddleware branch from 2cdcf45 to 378a820 Compare May 19, 2023 05:57

Add back and deprecate createAsyncMiddleware

3fb1a7c

rekmarks changed the title ~~Remove createAsyncMiddleware~~ Allow middleware to be async, deprecate createAsyncMiddleware May 19, 2023

rekmarks requested a review from Gudahtt May 19, 2023 20:32

rekmarks added 4 commits May 19, 2023 13:45

Update createAsyncMiddleware docstring

a9028af

Reorder some stuff

afd85f0

Undo unintended change to scaffold middleware

36d91ad

Revert "Undo unintended change to scaffold middleware"

6d5ddc6

We actually need this to please the linter. This reverts commit 36d91ad.

rekmarks commented May 19, 2023

View reviewed changes

Merge branch 'main' into remove-createAsyncMiddleware

49c1d95

legobeat mentioned this pull request Oct 12, 2023

Migrate json rpc engine repo MetaMask/core#1804

Closed

3 tasks

Merge branch 'main' into remove-createAsyncMiddleware

252827c

mcmire mentioned this pull request Oct 16, 2023

RFC: Remove next callbacks #83

Draft

mcmire reviewed Oct 16, 2023

View reviewed changes

MetaMask locked and limited conversation to collaborators Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow middleware to be async, deprecate createAsyncMiddleware #81

Allow middleware to be async, deprecate createAsyncMiddleware #81

rekmarks commented Mar 27, 2021 •

edited

Loading

Gudahtt left a comment

kumavis commented Apr 12, 2021

kumavis commented Apr 12, 2021

kumavis commented Apr 12, 2021

rekmarks commented May 17, 2021

rekmarks commented May 18, 2021

rekmarks May 19, 2023

mcmire Oct 16, 2023

mcmire left a comment

mcmire Oct 16, 2023

mcmire Oct 16, 2023

mcmire Oct 16, 2023

mcmire Oct 16, 2023

mcmire Oct 16, 2023

mcmire Oct 16, 2023

MajorLift commented Nov 7, 2023

	* @deprecated As of version 7.1.0, middleware functions can be asnync. This
	* @deprecated As of version 7.1.0, middleware functions can be async. This

Allow middleware to be async, deprecate createAsyncMiddleware #81

Are you sure you want to change the base?

Allow middleware to be async, deprecate createAsyncMiddleware #81

Conversation

rekmarks commented Mar 27, 2021 • edited Loading

Gudahtt left a comment

Choose a reason for hiding this comment

kumavis commented Apr 12, 2021

kumavis commented Apr 12, 2021

kumavis commented Apr 12, 2021

rekmarks commented May 17, 2021

rekmarks commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorLift commented Nov 7, 2023

rekmarks commented Mar 27, 2021 •

edited

Loading