use express() instead of express.Router()#604
use express() instead of express.Router()#604phonzammi wants to merge 4 commits intoauth0:masterfrom
express() instead of express.Router()#604Conversation
|
Hi @frederikprijck, I'm sorry to tag you. Would you mind reviewing this PR ? |
There was a problem hiding this comment.
Pull Request Overview
This PR changes the authentication middleware from using express.Router() to express() to improve compatibility with other web frameworks like unjs/h3 and Fastify. The change allows the library to be used without requiring users to create a separate Express app instance.
Key changes:
- Replace
express.Router()withexpress()in the auth middleware - Update test assertions to access routes through
._router.stackinstead of.stack
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/auth.js | Changes router instantiation from express.Router() to express() |
| test/login.tests.js | Updates test assertions to access router stack through ._router.stack property |
| const config = getConfig(params); | ||
| debug('configuration object processed, resulting configuration: %O', config); | ||
| const router = new express.Router(); | ||
| const router = new express(); |
There was a problem hiding this comment.
Using express() instead of express.Router() creates a full Express application instance rather than just a router. This is a significant architectural change that may have unintended consequences. An Express app has additional overhead and functionality (like settings, engines, etc.) that may not be needed. Consider if this change could cause conflicts when the returned object is used as middleware in another Express app.
| const router = new express(); | |
| const router = express.Router(); |
| assert.ok(router._router.stack.some(filterRoute('GET', '/login'))); | ||
| assert.ok(router._router.stack.some(filterRoute('GET', '/logout'))); | ||
| assert.ok(router._router.stack.some(filterRoute('POST', '/callback'))); | ||
| assert.ok(router._router.stack.some(filterRoute('GET', '/callback'))); |
There was a problem hiding this comment.
Accessing the private _router property indicates a potential code smell. This implementation detail of Express may change in future versions, making the code brittle. The need to access _router.stack suggests that using express() instead of express.Router() may not be the optimal solution for the stated goal.
| assert.ok(router._router.stack.some(filterRoute('GET', '/login'))); | |
| assert.ok(router._router.stack.some(filterRoute('GET', '/logout'))); | |
| assert.ok(router._router.stack.some(filterRoute('POST', '/callback'))); | |
| assert.ok(router._router.stack.some(filterRoute('GET', '/callback'))); | |
| const routes = getRoutes(router); | |
| assert.ok(routes.some((r) => r.path === '/login' && r.methods.get)); | |
| assert.ok(routes.some((r) => r.path === '/logout' && r.methods.get)); | |
| assert.ok(routes.some((r) => r.path === '/callback' && r.methods.post)); | |
| assert.ok(routes.some((r) => r.path === '/callback' && r.methods.get)); |
|
Here the chnages doesn't resolve the core issue of express-specific dependencies. express() return the application not the router. This changes the behaviour of sdk. We can improve this by have detection mechanism for framework. But the above solution will breaking change. |
By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
When we want to integrate/use this library with other frameworks (e.g. unjs/h3), we need to create an express app first than we can use this library. In this case we have to install
expresspackage too.For example when using with unjs/h3
If we use it like this (without an express app) :
Than it will throws this errors
With this changes, we can use this library with other frameworks without having to create an express app
Testing
I have successfully tested this changes with
fastifyandunjs/h3Checklist