lukeed / trouter

:fish: A fast, small-but-mighty, familiar fish...errr, router*
MIT License
634 stars 23 forks source link

Can we add remove method to this library? #3

Closed iinsta closed 5 years ago

lukeed commented 6 years ago

Hey, what do you mean? There's already a app.delete method attached. Trouter automatically attaches all http.METHODS to itself.

You can see the test output here: https://travis-ci.org/lukeed/trouter/jobs/327613582#L609

iinsta commented 6 years ago

I mean add a remove method like unsubscribe to subscribe:

    add(method, pattern, handler) {
        // Save decoded pattern info
        this.routes[method].push(parse(pattern));
        // Save route handler
        this.handlers[method][pattern] = handler;
        // Allow chainable
        return this;
    }
    remove(method, pattern, handler) {
      // remove added method
   }

https://github.com/lukeed/trouter/blob/master/lib/index.js#L16-L23

lukeed commented 6 years ago

Ah, gotcha. May I ask the purpose/reason for doing this?

iinsta commented 6 years ago

Then we can have config to generate and disable router in fly, not restart

lukeed commented 6 years ago

You can't have a dynamic route handler? Sorry, I've never seen this need before

iinsta commented 6 years ago

Dynamic route can't process this kind:

app.use('/web1/images', serve('./web1/assets/images'))
app.use('/web2/images', serve('./web2/assets/images')) 
// I know how to add `'/web3/images'`
// but how to disable `'/web2/images'` on fly?
AsynchronousToddler commented 6 years ago

I'm honestly not a fan of reloading code on the fly, and I'm not sure what kind of use case would enable/disable routes dynamically.

lukeed commented 5 years ago

Closing this as I still see no need for this in a Node.js server context.

I'm open to being convinced otherwise, but haven't been yet 😉Can reopen for discussion at that time.

Thanks!

bugs181 commented 5 years ago

Sorry, I have to disagree. Just because YOU don't see a need only hints at lack of imagination. I was actually coming here with the intention of replacing my custom router scenario on a much bigger project, needing this functionality. My use case is extremely simple, private (recursive?) routing capabilities.

  1. Project loads all routes from a config (just as posted)
  2. Config is processed into a router to do transformations
  3. Some of those routes (via transformations) may need to be "private" so they go into a private router instance.
  4. Router (trouter) would then be sent to protocols to be served up to the public.

That in mind, I'd need .delete or .remove (don't really have a preference) and a .forEach method to accomplish my goals.

lukeed commented 5 years ago

@bugs181 Sounds like you need two routers.

const privRouter = new Trouter();
const publRouter = new Trouter();

myRoutes.forEach(obj => {
  let router = obj.isPrivate ? privRouter : publRouter;
  router.add(obj.method, obj.pattern, ...obj.handlers);
});
bugs181 commented 5 years ago

The problem is that I would have to have knowledge up front about which routes to put in what router. This is impossible with my scenario as transformer plugins can mark routes as being private. That would mean, I would have to duplicate trouter work - just to provide this functionality, that could be solved with a simple .remove function.

lukeed commented 5 years ago

Well, Trouter is a class. You can import it and add your own method to the prototype.

Or not use Trouter at all and roll your own like I did.

This won't be added to Trouter, NOT because I lack imagination (rude, thanks) but because I'm purposefully discouraging poor server design. Popping off routes will deoptimize the runtime of your server. It may not matter with low-traffic side projects, but it certainly matters with high traffic/stress.

You have options.

bugs181 commented 5 years ago

I never said the server started before the router was able to process the routes. I apologize that you think my constructive criticism is rude. I've seen so many projects/libraries fail because authors, much like yourself think they know all use-cases. Please do take this as constructive feedback.

You also making the assumption that this is somehow "poor server design" - because you don't understand the project stack I'm dealing with - could also be construed as rude and insulting.

The idea is to have multiple trouter instances with a coordination mechanism about where to apply what routes (that can't be known until transformation runtimes happen). In addition to that, the project I'm working on is much like how Babel works - you can compile your routes (and transformations) for optimizations. Nothing about this is "poor design" - unless you are saying that the thousands of other libs that have any sort of pre-compilation or post-compilation hooks (such as that of babel) are designed poorly.

lukeed commented 5 years ago

I understand fully. This is the implementation I've chosen to share publicly and maintain. You don't have to use it.

Good luck with your project