fastify / fastify-express

Express compatibility layer for Fastify
MIT License
244 stars 25 forks source link

@fastify-express not working inside a plugin #106

Open fox1t opened 1 year ago

fox1t commented 1 year ago

Prerequisites

Fastify version

4.x.x

Plugin version

2.3.0

Node.js version

16.18.1

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

Ventura 13.0

Description

Every express middleware (regardless of being an app, router, middleware, or route) mounted inside a fastify plugin isn't working and returns 404. I've added encapsulation tests to mimic the application.test.js file, and the only test passing is: Should not expose the express app on the root fastify instance when registered inside a plugin.

To make this clearer, this is not working:

fastify.register(expressPlugin).after(function () {
    fastify.register(function (instance, opts, done) {
      const express = Express()
      express.use(function (req, res, next) {
        res.setHeader('x-custom', true)
        next()
      })

      express.get('/hello', (req, res) => {
        res.status(201)
        res.json({ hello: 'world' })
      })
      instance.use(express)
      done()
    })
  })

Steps to Reproduce

Clone https://github.com/fox1t/fastify-express and run npm i && npm run test:unit ./test/encapsulation.test.js

Expected Behavior

As far as the docs say, it should be possible to add express apps, routers, middleware, and routes inside a plugin. Ref: https://github.com/fox1t/fastify-express#encapsulation-support

mcollina commented 1 year ago

Good spot. The problem is that the onRequest hook is not firing in your case. Adding a 404 handler within the plugin would do the trick.

fox1t commented 1 year ago

Yes, I observed the same about the onRequest not being called. Do you think we can call instance.setNotFoundHandler inside the onRegister hook to add a dummy handler? ref: https://github.com/fastify/fastify-express/blob/master/index.js#L83

fox1t commented 1 year ago

This is enough to make it work:

function onRegister (instance) {
    instance.setNotFoundHandler()
    const middlewares = instance[kMiddlewares].slice()
    instance[kMiddlewares] = []
    instance.decorate('express', Express())
    instance.express.disable('x-powered-by')
    instance.decorate('use', use)
    for (const middleware of middlewares) {
      instance.use(...middleware)
    }
  }

The issue is, of course, that now it is impossible to call setNotFoundHandler on the root instance if the plugin's prefix is /. Maybe we can introduce a check: if the user mounts the express plugin on a prefix different than / we can call instance.setNotFoundHandler(), otherwise, not. I like neither saying to the user to handle this manually nor to add this if check. Any Ideas?