pillarjs / router

Simple middleware-style router
MIT License
409 stars 102 forks source link

feat: removeAllRoutes public method #93

Closed robertsLando closed 7 months ago

robertsLando commented 4 years ago

This method allows to clear routes stack. As discussed here: https://github.com/expressjs/express/issues/4296

robertsLando commented 4 years ago

In my case, I have some plugins that a user can add to the running server, those plugins can be customized and can also add apis (routes) to the server. When a plugin is refreshed I need to remove all eisting routes for that plugin and add back the new updated routes.

would a use expect that calling this would reset the registered parameters

I don't know what you mean as I didn't checked all the source code. By consider my use case do you think that is necessary? Please let me know if I need to add something or feel free to edit my pr.

I added a test, let me know if it's good for you

dougwilson commented 4 years ago

Based on your use case description, I'm not sure, but it sounds like your plugin should be returning a new router each time it is "refreshed", which would make this addition not necessary, right? What does this addition add or make possible vs returning a new router fresh for each refresh?

For example, would a user who removes all the routes expect that the router kept the prior strict routing behavior? What if the user wanted to remove all the routes in order to add new routes with a different strict behavior? I guess the same could be asked about the case-sensitivity feature. Those switches are of course aspects of how the routes are interpreted, same as for the params.

For example, say you have a router with some routes like /user/:id. You get a reference to that router and want to remove all the routes, so call this new method. You then add new ones like /obj/:id and of course call .param to define what the :id will look like... but it's different than that user one and now conflicts with the previous definition.

So the above scenario may be confusing for how one would use this new method as it's currently (as of the comment) implemented, as there are still aspects of the previous routes still lingering on the router even after calling this method. I of course understand this method is called "removeAllRoutes", but without docs in this PR it's still hard for me to understand what that is expected to do.

Of course, most importantly of all the questions, is the first couple in the comment if you can get to them. Many times having multiple ways to do the same thing leads to user confusion, but also if there is already a way to do this and it is more configurable/capable, it seems adding a partial feature may not be the best long-term route to take, but that's what I'm trying to figure out 😂 .

I think your use-case outline above is leaving a lot to be desired, as I think you are accidentally excluding aspects that is important for us to know here. For example, you say "plugins" but there is no such thing in this module or Express, so I really don't know what that means exactly, which makes following your use-case quite difficult.

jonchurch commented 4 years ago

Can you provide us a sample app that would benefit from this feature? Maybe a comparison w/ how things are now, and how you would like them to work with the inclusion of this feature.

This is an interesting feature, but it also feels a lot like state management which I'm wary of. The route stack is of course already a type of application state, and Express is very unopinionated so if you want to manage routes as state that's up to the user. But something feels off about providing an API that encourages users to treat the route stack like state that can be wiped with a single function call.

robertsLando commented 4 years ago

Based on your use case description, I'm not sure, but it sounds like your plugin should be returning a new router each time it is "refreshed", which would make this addition not necessary, right? What does this addition add or make possible vs returning a new router fresh for each refresh?

This allows me to keep using the same router instance without creating another one or use the trick you mentioned here

So the above scenario may be confusing for how one would use this new method as it's currently (as of the comment) implemented, as there are still aspects of the previous routes still lingering on the router even after calling this method. I of course understand this method is called "removeAllRoutes", but without docs in this PR it's still hard for me to understand what that is expected to do.

I never used the param so I need to check what it is used but if that is the behaviour of course also that should be 'cleared' someway giving you a clean router instance

I think you are accidentally excluding aspects that is important for us to know here.

That is sure as I don't know ROuter as you and I didn't checked all the source code :smile:

For example, you say "plugins" but there is no such thing in this module or Express, so I really don't know what that means exactly, which makes following your use-case quite difficult.

Yes in that case plugins is something to identify a 'program' that is added on runtime to my webserver, in poor words that is a plugin for me. I pass the router as a parameter in this "plugin" constructor and the plugin can use that instance to add new routes. But If I reload a plugin some routes could persist without clearing old ones.

dougwilson commented 4 years ago

This allows me to keep using the same router instance without creating another one or use the trick you mentioned here

So I think your view of it as "trick" is not correct -- it it no trick at all, just code to implement what you want. There is nothing wrong with it.

I think you are accidentally excluding aspects that is important for us to know here.

That is sure as I don't know ROuter as you and I didn't checked all the source code 😄

Oh, I think I may have miss-spoke here. I'm referring to excluding aspects of your use-case us, and important for us to know about your use-case. We understand you don't know the router code, and that is what we want to help with, but are still unclear on your use-case.

I think what @jonchurch said in https://github.com/pillarjs/router/pull/93#issuecomment-634757459 would be helpful for sure 👍

robertsLando commented 4 years ago

This is an interesting feature, but it also feels a lot like state management which I'm wary of. The route stack is of course already a type of application state, and Express is very unopinionated so if you want to manage routes as state that's up to the user. But something feels off about providing an API that encourages users to treat the route stack like state that can be wiped with a single function call.

Keep it like a proposal guys, I saw some questions around stackoverflow and plugins like https://www.npmjs.com/package/express-remove-route that would like someting to manage routes. In my case this is enought but could open a new way of features around routes management (add/remove specific ones for example)

wesleytodd commented 7 months ago

I think it sounds like this was agreed to be better done with a new router instance. So I am going to close this. If you want to open a new issue to discuss alternatives please do. One alternative I think worth discussing is showing folks how to easily use a new router to achieve this behavior.