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

mounting into an express app #846

Closed
bttmly opened this issue Oct 28, 2016 · 16 comments
Closed

mounting into an express app #846

bttmly opened this issue Oct 28, 2016 · 16 comments

Comments

@bttmly
Copy link

bttmly commented Oct 28, 2016

I'm exploring migrating a large production Express app to koa v2 (so all of this applies to the promise API of v2). The koajs.com docs say for app.callback():

"You may also use this callback function to mount your koa app in a Connect/Express app."

However, it seems like you must mount a koa app at the "bottom" of your Express app since koa will never call next() to yield control back to express (like in the 404 case). I can't see an obvious work around for this currently. If the function returned by callback returned the promise, I think something like the following would work:

const koaCallback = koaApp.callback();
expressApp.use(function (req, res, next) {
  koaCallback(req, res)
    .then(function () {
      // not sure exactly how to detect this but seems doable
      if (!koaSentResponse(res)) {
        next() // let express keep processing this request
      }
   });
});

Perhaps somewhat related -- what's the use case for setting ctx.respond to false?

@PlasmaPower
Copy link
Contributor

You probably want res.finished.

I'm not quite sure what ctx.respond = false would be used for, as a custom responder at the beginning of the chain should do the same job. It shouldn't be used, it's not officially supported and it's bound to break stuff.

@PlasmaPower
Copy link
Contributor

I think the docs for you're looking at would assume that Koa would be used to handle a subdirectory like /api/... or similar, as opposed Koa being the default and Express filling in the gaps.

@bttmly
Copy link
Author

bttmly commented Oct 28, 2016

Huh. The ctx.respond = false line even has a comment saying "allow bypassing koa". Just for some background, the use case here is I want to move routes in an existing application over to koa incrementally, without affecting clients. I could, I suppose, have a list of routes that should go to koa and take care of getting them there manually via an express/connect middleware, but that ends up duplicating a lot of the routing logic.

@PlasmaPower
Copy link
Contributor

@nickb1080 Yep, I figured so. I don't think you want ctx.respond = false here though, I think you're initial solution is good with if (!res.finished) (instead of if (!koaSentResponse(res))). You could also manually assign routes to Koa of course, but that might be a pain depending on how many routes you have.

@PlasmaPower
Copy link
Contributor

Wait, I just realized you're going to get a 404 response from Koa of course. I'll see how that can be prevented.

@PlasmaPower
Copy link
Contributor

It looks like ctx.respond = false might be useful after all. Try making your first middleware return next().then(() => ctx.respond = (ctx.body == null)); or similar (double equal sign as undefined == null but '' != null).

@bttmly
Copy link
Author

bttmly commented Oct 28, 2016

Yeah I that'll work for avoiding the automatic 404 but I still see no way of calling the next provided by express/connect that would give control back to the enclosing app. In particular, callback returns a function of signature (req, res) -- the (possible) next provided by express/connect isn't even referenced. If it returned the promise I think we'd be in business though.

I'm experimenting with a hacky workaround that subclasses the Koa application class. It's sort of a pain because the respond function is private to that module so I need to inline the whole thing into the subclass.

@PlasmaPower
Copy link
Contributor

@nickb1080 Ooh, that's a tricky one, I thought handleRequest returned the promise. I'll make a PR for that now.

PlasmaPower added a commit to PlasmaPower/koa that referenced this issue Oct 28, 2016
Without this, there's no way of hooking into after Koa is done with the response. This can be extremely important if used with `ctx.respond = false`, as in koajs#846 (see there for use case). While this is technically a breaking change, I don't expect it to break anything.
@PlasmaPower
Copy link
Contributor

In case you didn't get a notification, I opened #847 for this.

@jonathanong
Copy link
Member

jonathanong commented Nov 26, 2016

@PlasmaPower has a PR open. if anyone else would like to put in their 2 cents, that would be great!

/cc @nickb1080

@PEM--
Copy link

PEM-- commented Dec 2, 2016

Wow, that would solve a lot of integration issues and smoothen transitions to Koa.

@bttmly
Copy link
Author

bttmly commented Dec 2, 2016

The test case for PR #847 appears to address all my concerns!

@PlasmaPower
Copy link
Contributor

I'm glad! @jonathanong anything holding it up? The name of the variable might not be great, but it's fine IMO. I have tests for it too.

@fl0w
Copy link
Contributor

fl0w commented Dec 3, 2016

Maybe just app.middleware()? IMO it's kind of related to current naming scheme of app.callback() and app.listen(). Highly subjective of course.

@PlasmaPower
Copy link
Contributor

@fl0w Hmm, maybe. The difference of course is that it doesn't accept ctx, next, it accepts req, res.

@bttmly
Copy link
Author

bttmly commented Mar 9, 2017

This is resolved in e6539e1

@bttmly bttmly closed this as completed Mar 9, 2017
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

5 participants