matthiaaas / express-file-routing

Flexible file-based routing for Express (like Next.js + additional features)
https://www.npmjs.com/package/express-file-routing
MIT License
78 stars 12 forks source link

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

Open R1D3R175 opened 2 weeks ago

R1D3R175 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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.

R1D3R175 commented 1 week ago

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:

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 commented 1 week ago

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 commented 1 week ago

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).