-
Notifications
You must be signed in to change notification settings - Fork 152
use express()
instead of express.Router()
#604
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
base: master
Are you sure you want to change the base?
Conversation
Hi @frederikprijck, I'm sorry to tag you. Would you mind reviewing this PR ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.stack
instead 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 |
@@ -23,7 +23,7 @@ const enforceLeadingSlash = (path) => { | |||
const auth = function (params) { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
Copilot uses AI. Check for mistakes.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)); |
Copilot uses AI. Check for mistakes.
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
express
package 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
fastify
andunjs/h3
Checklist