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

Adds ability to list routes for lucky routes command #64

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

matthewmcgarvey
Copy link
Member

Fixes #57

Lucky stores its own routes array and it is only for the lucky routes CLI command. By adding this functionality here, we remove the need for that. The basic logic of this code is that a fragment returns a list of any methods to payload that it has along with its path. Beyond that, it cycles through any sub-fragments it might have and does the same for those and prepends its own path to any results received from it. That ultimately bubbles up to a single list of routes. Only thing we do beyond that is to convert the list of PathPart's into a string. We filter out any of the blank path parts that are at the start and join with "/" and make sure to prepend one as well. It gets a little funky when query params are passed in, but Lucky doesn't do that at all so it shouldn't be a problem. The problem there is that it makes a ton of routes. With query param, without, not including if there's more than one.

Lucky router with duplicate routes array: https://github.com/luckyframework/lucky/blob/main/src/lucky/router.cr
CLI command: https://github.com/luckyframework/lucky/blob/main/tasks/routes.cr

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I think for that Ameba error, we can ignore on this PR. For the other shards, I'm slowly bringing the min crystal version up to 1.8 since 1.13 will be out in the next month. Then ameba can be bumped to 1.5 (since 1.6 doesn't play nice with Lucky). But all in a separate PR.

Thanks for adding this in! That'll be a little nice memory savings

@matthewmcgarvey
Copy link
Member Author

@jwoertink I don't have a lucky app at the moment to verify this. I tested it using the lucky action spec. Would you be able to try this out in your app by using this branch and also changing the lucky routes task to use it? Just want to make sure we don't lose routes we had before or add ones we don't need. In my test, I had the updated lucky routes task filter out "HEAD" requests because those are added by the router. I don't know if people would manually create those, though.

@jwoertink
Copy link
Member

hmm... So lucky routes still works like normal since that code doesn't change, but LuckyRouter::Matcher(Lucky::Action.class).new.list_routes just returns an empty array. I guess that makes sense since the LuckyRouter doesn't really know about my app yet. I assume we would still need some way to tell the router what has been registered?

@matthewmcgarvey
Copy link
Member Author

It will be easier to test once I have the PR up for the lucky changes. Going to merge this and make that update later today, probably.

@matthewmcgarvey matthewmcgarvey merged commit d1d5e78 into main Jun 12, 2024
3 of 4 checks passed
@matthewmcgarvey matthewmcgarvey deleted the listing-routes branch June 12, 2024 20:15
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.

Provide way to access all stored routes, methods, and payload
2 participants