koajs / discussions

KoaJS Discussions
2 stars 2 forks source link

ctx.originalUrl invalid when used as express route #19

Open matthieusieben opened 5 years ago

matthieusieben commented 5 years ago

When using a Koa app as an express middleware, with a path prefix, the ctx.originalUrl will not be valid and will not contain the path prefix:

const expressApp = express()
const koaApp = new Koa()
const koaRouter = new Router()

koaRouter.get('/test', async (ctx, next) => {
  console.log(ctx.originalUrl) // will not contain "/api"
  ctx.body = { foo: "bar" }
})

koaApp.use(koaRouter.routes())
expressApp.use('/api', koaApp.callback())

This is due to the fact that, behind the scene, the express router will manipulate req.url when the request is being processed by router middleware.

I would suggest that Koa does something similar to what express does when it is itself used as a middleware, meaning preserve the original originalUrl: https://github.com/expressjs/express/blob/f3fa758af9664526ee18c58764590e48f559e6ae/lib/router/index.js#L172

My suggestion is to change the following line of lib/application.js:

  context.originalUrl = request.originalUrl = req.url;
  // to
  context.originalUrl = request.originalUrl = req.originalUrl || req.url;

Now one can argue that this would be a breaking change, especially because any route defined in koa will need to reflect the prefix again. In my example above, the routes in the router would have to be defined as koaRouter.get("/api/test", ...). That is true, but we also need to consider that the current implementation already breaks the request URL related getters.

matthieusieben commented 5 years ago

A less breaking solution would be to add an option to restore the originalUrl when generating a callback:

  callback(options = {}) {
    const fn = compose(this.middleware)

    if (!this.listenerCount('error')) this.on('error', this.onerror)

    const handleRequest = (req, res) => {
      if (options.restoreUrl) req.url = req.originalUrl || req.url
      const ctx = this.createContext(req, res)
      return this.handleRequest(ctx, fn)
    }

    return handleRequest
  }
matthieusieben commented 5 years ago

My current "hack" is to monkey patch the callback():

  const koaMiddleware = koaApp.callback()

  expressApp.use('/api', (req, res) => {
    req.url = req.originalUrl || req.url
    handleRequest(req, res)
  })
matthieusieben commented 5 years ago

A more complex (?) alternative would be to make use of the req.baseUrl that the express router adds, when computing the originalUrl.

fl0w commented 5 years ago

I don't see the point, why would you run Koa as a middleware for express? In addition, modify Koa source for an unsupported/unintended use-case?

matthieusieben commented 5 years ago

Thanks for your answer.

I have several use-cases for this:

1) At my company we currently use Express. I want to push Koa because I think it is a more modern and overall better way to implement the middleware pattern. But my company cannot afford a full re-write. So the only solution is to push Koa as the replacement, and slowly migrate endpoints 1 by 1 to Koa. Doing this requires to use Koa as an Express middleware.

2) Some libraries use Express under the hood. For example, Nuxt.js uses Express for its serverMiddlewares. As I said, I prefer using Koa to implement my middlewares.

3) This is not coming from me, really. Here is an extract of Koa doc on koajs.com:

app.callback() Return a callback function suitable for the http.createServer() method to handle a request. You may also use this callback function to mount your Koa app in a Connect/Express app.

The project's own doc recommend using callback() to mount a koa app into an express app. But this integration is currently broken due to this originalUrl problem.

Now I understand that you don't want to make changes to the Application's createContext() methods. And, to be honest, I don't think it is the right way either.

If there is one place where Koa's code should care about integration with another lib, it should be in the one place build for that purpose: the callback() method. Of course you can argue that this can be done in user-land, or even in yet another (one liner) NPM module. But then, you should remove any mention to Express from the doc, or at least mention that the integration with Express is broken in some cases. And I don't think that that is going to help Koa become a replacement for Express.

Frondor commented 4 years ago

Hello! Is this still an issue? I see the source code looks like the changes suggested are already implemented.

I'm actually experiencing the same limitation while doing a forward rewrite.