fastify-express icon indicating copy to clipboard operation
fastify-express copied to clipboard

@fastify-express not working inside a plugin

Open fox1t opened this issue 2 years ago • 3 comments
trafficstars

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] I have searched existing issues to ensure the bug has not already been reported

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

fox1t avatar Dec 20 '22 10:12 fox1t

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.

mcollina avatar Dec 20 '22 10:12 mcollina

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 avatar Dec 20 '22 11:12 fox1t

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?

fox1t avatar Dec 20 '22 11:12 fox1t