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

[Feature Request] Add support for "global" middlewares #35

Open
R1D3R175 opened this issue Jun 18, 2024 · 5 comments
Open

[Feature Request] Add support for "global" middlewares #35

R1D3R175 opened this issue Jun 18, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@R1D3R175
Copy link

R1D3R175 commented Jun 18, 2024

As per #11, it would be nice to have the equivalent of the following (snippet of an hypothetical index.ts)

app.use('/api/database', middlewareFunction);

via some files/folder structure

Why?

Right now, here's how my index.ts looks like:

app.use('/api/database', isLogged);
app.use('/api', await router({directory: path.join(__dirname, 'src', 'routes')}));

app.use(express.static(path.join(__dirname, 'public')));
app.use('*', async (request, response) => {
	response.sendFile(path.join(__dirname, 'public', 'index.html'));
});

app.use((error: Error, request: Request, response: Response, next: NextFunction) => {
	const prefix = `[${new Date().toISOString()}] [${request.ip}] [${request.method}] [${request.url}]`;
	console.error(prefix);
	console.error(error);

	response.status(500).json({error: isProduction ? 'Internal server error' : error.message});
});

It works correctly, however, what if I rename the database folder? The middleware won't be registered anymore.
IMHO this shouldn't be happening since one of the main purpose of file routing should be hassle-free route managing.

Possible implementation

Like there is index.ts (i.e. src/routes/database/index.ts would correspond to /api/database according to my example), there could be a middleware.ts with content similar to an Handler matching all methods:

const middlewares = [
  isLogged, hasPermissions
];

export default middlewares;
@krishnabrq
Copy link

I think this would be a great feature to add considering that it would and reduce a lot of repetitive code result in better DX.

@matthiaaas
Copy link
Owner

matthiaaas commented Jun 20, 2024

Thank you for filing a feature request, @R1D3R175. A while ago, I decided against implementing such a feature, but I will reconsider this decision in the near future. Meanwhile, I encourage everyone interested in this to upvote the head issue so I can gauge the demand for this feature.

I might favor the implementation to not create a separate middleware.js file, but rather a named export like:

// routes/api/database
...

export const get = () => {}

export const put = () => {}

export const middlewares = [
  isLogged, hasPermissions
]

But I am open for a discussion.

@matthiaaas matthiaaas added the enhancement New feature or request label Jun 20, 2024
@R1D3R175
Copy link
Author

Thank you for filing a feature request, @R1D3R175. A while ago, I decided against implementing such a feature, but I will reconsider this decision in the near future. Meanwhile, I encourage everyone interested in this to upvote the head issue so I can gauge the demand for this feature.

I might favor the implementation to not create a separate middleware.js file, but rather a named export like:

// routes/api/database
...

export const get = () => {}

export const put = () => {}

export const middlewares = [
  isLogged, hasPermissions
]

But I am open for a discussion.
First of all, thank you for considering my feature request. I can see why you would prefer implementing it as a named export and, it's indeed better for seeing which middlewares are bound certain route groups. However I think we should define the "middleware bounding priority" and the design to assign such "priority"; I'll explain what I mean by using an example scenario.

Example Scenario

Suppose we have a Twitter-like app that has the following endpoints:

  • /api/database, interacts with the database for storing or retrieving data, so we would have /user and /post.
  • Frontend routes such as /post that call the database API to fetch/insert posts and /user to get information about a user.
  • Generic routes to handle registration and login.

In our Twitter-like app, an user has to be logged in for seeing other posts in their feed, so we would use the following middlewares to protect the interested routes: isLogged, hasPermissions.
Let's now say that a user has decided to keep their profile private, we would have to add some logic to the hasPermissions endpoint. You can see how the hasPermissions is a pretty bad design since it breaks the single responsibility principle.

Because of that, we remove the hasPermissions middleware and break it into multiple middlewares such as canViewUser, canViewPost; we apply these using the already existing syntax.
But we still have the isLogged middleware on the parent folder. In which order would the middlewares be executed?

Consideration

If we view the routes as a tree, it's quite obvious that the parent middlewares gets "bound" before the child middleware. But what if we want to make an exception?
Let's say that I have the /api/database protected by the isLogged middleware since all APIs require a user id, however we also have the /user route which handles checking if an user exists or inserting one, thus, we don't want these routes to be protected.

Conclusion

What I'm saying could be quite obvious or kinda stupid, however, IMHO these are the things that could introduce the most bugs if not well-defined beforehand.

@matthiaaas
Copy link
Owner

Thank you for the insight @R1D3R175. I think it takes some more clarification for me. My idea of a named middlewares export would be that those middlewares would only apply to the method exports inside that route file, not for sibling or child routes. For example:

// routes/api/database.ts or routes/api/database/index.ts
export const get = () => {}

export const post = () => {}

export const middlewares = [isLogged, canViewDatabase]
// routes/api/database/user.ts
export const get = () => {}

export const put = [canUpdateUser, () => {}]

export const middlewares = [isLogged, canViewUser]

The named middlewares export in this concept would only apply these middlewares to the method exports defined in that route file.

So the example would be equivalent to the following snippet, where the middlewares are called in their order/priority of definition:

app.get("/api/database", isLogged, canViewDatabase, () => {})
app.post("/api/database", isLogged, canViewDatabase, () => {})

app.get("/api/database/user", isLogged, canViewUser, () => {})
app.put("/api/database/user", isLogged, canViewUser, canUpdateUser, () = {})

In this concept, the middlewares export is just a shorthand for applying middlewares to all method exports, so it's not a truly global middleware.

For a truly global middleware that also matches all its subsequent child routes, the idea of a dedicated file feels like the right way.

@R1D3R175
Copy link
Author

R1D3R175 commented Jun 22, 2024

Sounds good to me @matthiaaas. Thanks for considering this feature request. At this point I think we got everything all settled (i.e. both global and per route middleware boundings), IMHO a good starting point could be updating the docs accordingly and see if things make sense; if they do, it's a matter of implementing the feature according to what's written in the docs (something TDD, except it's docs driven).

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

No branches or pull requests

3 participants