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

@fastify-express not working inside a plugin #106

Open
2 tasks done
fox1t opened this issue Dec 20, 2022 · 3 comments
Open
2 tasks done

@fastify-express not working inside a plugin #106

fox1t opened this issue Dec 20, 2022 · 3 comments

Comments

@fox1t
Copy link
Member

fox1t commented Dec 20, 2022

Prerequisites

  • I have written a descriptive issue title
  • 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

@mcollina
Copy link
Member

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
Copy link
Member Author

fox1t commented Dec 20, 2022

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
Copy link
Member Author

fox1t commented Dec 20, 2022

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?

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

No branches or pull requests

2 participants