json-rpc-engine icon indicating copy to clipboard operation
json-rpc-engine copied to clipboard

Allow middleware to be async, deprecate createAsyncMiddleware

Open rekmarks opened this issue 4 years ago • 6 comments

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.

rekmarks avatar Mar 27 '21 18:03 rekmarks

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 avatar Apr 12 '21 01:04 kumavis

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 avatar Apr 12 '21 01:04 kumavis

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

kumavis avatar Apr 12 '21 01:04 kumavis

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 avatar May 17 '21 20:05 rekmarks

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.

rekmarks avatar May 18 '21 17:05 rekmarks

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.

MajorLift avatar Nov 07 '23 17:11 MajorLift