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: add functions to add + remove dispatch routes #1585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artman41
Copy link

@artman41 artman41 commented Nov 1, 2022

Adds helper functions to insert & remove dispatch routes at runtime.

@artman41 artman41 force-pushed the feature/add_and_remove_routes branch from 3218156 to f68ded1 Compare November 1, 2022 23:34
@essen
Copy link
Member

essen commented Nov 2, 2022

This changes the route order though?

@artman41
Copy link
Author

artman41 commented Nov 2, 2022

@essen my thought behind it is that if you already had a match-all dispatch /[...] then any additional dispatches would fail to match as you'd fall into the first case - with that logic, it makes sense to add newer dispatches before the existing ones.

Happy to modify the implementation to put new dispatches at the end if you think that'd be better though

@essen
Copy link
Member

essen commented Nov 2, 2022

I question the approach. I think if something was to be added it would have to be something more general, like a function that traverses routes and allows inserting/removing routes at any point. Go through the list in order and allow returning [], one route tuple, or a list of routes tuples, and use that return value to update the list. Probably worth also adding a way to stop while keeping all subsequent routes as well.

@artman41
Copy link
Author

artman41 commented Nov 2, 2022

@essen that sounds a lot more involved than the easy MR I had in mind 😉

If you get chance to write up a bit of a detailed idea of what you mean then I'll give it a go 👍

@essen
Copy link
Member

essen commented Nov 7, 2022

I wonder if we want to operate on the dispatch list. It's probably better to work on the routes themselves. In a PoC farwest_router module 1 route may get compiled in multiple routes in the dispatch list. This makes it more difficult to write a function that preserves order. Operating on the cowboy_router/farwest_router routes is more appropriate not only in that case but also from a user point of view.

Perhaps in Cowboy 3.0 (which IMO should use the farwest_router by default and can keep cowboy_router for compat) the routes should be given directly along with the router module, and the dispatch list hidden internally. Cowboy would call compile itself. Then we could keep the routes around for retrieval and modifications and compile as needed. Then a function that goes over the routes would make sense.

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

Successfully merging this pull request may close these issues.

2 participants