Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we support async middleware factories? #74

Closed
giladgo opened this issue Apr 27, 2016 · 5 comments
Closed

Can we support async middleware factories? #74

giladgo opened this issue Apr 27, 2016 · 5 comments

Comments

@giladgo
Copy link

giladgo commented Apr 27, 2016

Hey,
Assuming this is our config:

...
"my-middleware": {
    "priority": 2,
    "module": {
        "name": "./foo.js",
        "method": "makeMiddleware"
    }
}
...

And this is foo.js:

module.exports = {
  makeMiddleware: function(callback) {
    // init code ...
    callback(function(req, res, next) {
      // whatever
    });
  }
}

As you can see, makeMiddleware is an asynchronous function, which yields to a callback. This can happen because sometime middleware initialization can do async stuff (like reading from a config file, database, etc.). Unfortunately, from looking at the docs and code, it seems that there is no support for async middleware factories, so the code above will fail.

Can we add support for that? 😄

@jasisk
Copy link
Member

jasisk commented Apr 27, 2016

There are two pieces of complexity here.

The first is minor and has to do with a bit of ambiguity. The current signature includes passing in an array of arguments. This signature would have to change to support a callback being programmatically appended. Not a huge deal but, off the top of my head, I cannot come up with a new signature that I like to support both. Love to hear if you have any thoughts.

The second complexity is a bit of a doozy. Express has no concept of asynchronous middleware registration. If you take a peek at how express registers functions into the continuation, it's wholly synchronous. The way things work right now, the moment you register meddleware into your app, everything else done by meddleware happens synchronously. This means that the order of middleware registration is predictable. Introducing async resolution into a pipeline that has no concept of async would mean registration order would no longer be predictable.

In other words, app.use(meddleware(config), otherMiddleware) could result in otherMiddleware being before, after, or somewhere in the middle of meddleware-defined middlewares.

@jasisk
Copy link
Member

jasisk commented Apr 27, 2016

As for how you can accomplish this already:

The meddleware config can be resolved asynchronously, which would then be passed to meddleware. This is exactly how kraken uses it. To perform a similar action in kraken, you could perform your various async steps in the onconfig method you can pass into the kraken config. You could then set the values resulted by the async steps into the config as middleware arguments.

@giladgo
Copy link
Author

giladgo commented Apr 27, 2016

Oh wow, haven't thought of that, good point 😄
I guess I can try to use onconfig for now.
Thanks!

@giladgo giladgo closed this as completed Apr 27, 2016
@jasisk
Copy link
Member

jasisk commented Apr 27, 2016

Sure. Let me know if an example or two would help. 😀

@jknight12882
Copy link

@jasisk I wouldn't mind seeing a couple of examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants