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

Add route_prefix macro for prefixing/namespacing routes #1087

Closed
paulcsmith opened this issue Apr 6, 2020 · 9 comments · Fixed by #1121
Closed

Add route_prefix macro for prefixing/namespacing routes #1087

paulcsmith opened this issue Apr 6, 2020 · 9 comments · Fixed by #1121
Labels
feature request A new requested feature / option

Comments

@paulcsmith
Copy link
Member

#834 (comment)

abstract class ApiAction < Lucky::Action
  route_prefix "/api/v1"
end

Would prefix routes that inherit from this class with /api/v1 so you don't have to do it for every single route

@paulcsmith paulcsmith added the feature request A new requested feature / option label Apr 6, 2020
@wout
Copy link
Contributor

wout commented Apr 6, 2020

That would be perfect! Will param interpolation work there as well? For example:

abstract class ShopAction < Lucky::Action
  route_prefix "/shops/:handle"
end

@paulcsmith
Copy link
Member Author

@wout yes! It’ll basically copy paste the prefix as is. So anything you’d do in get/post/etc would work in route prefix

@rnice01
Copy link
Contributor

rnice01 commented Apr 16, 2020

I'd like to work on this, should this macro go in the routable module?

@jwoertink
Copy link
Member

@rnice01 Yeah, I think that's what I'd probably put it. That module is included in all of the actions already.

@paulcsmith
Copy link
Member Author

@rnice01 Thanks for contributing! This will almost certainly require some macros. They can be a bit tricky to work with so if you need any help feel free to ping me

@rnice01
Copy link
Contributor

rnice01 commented Apr 21, 2020

Hey @paulcsmith , I'm a bit stuck on this one 😅 . So I'm not sure the best way to accomplish this but here's what I have so far. I'm trying to define a method that will return the prefix if the prefix_path macro is called. Not sure if this is the best way to do it. Where I'm stuck is trying to call the method in the add_route macro.

`
# Returns the route prefix
# set from the route_prefix macro
def prefix_for_path()
end

# Defines the prefix for all
# of the action's methods
macro prefix_path(prefix)
def prefix_for_path()
prefix
end
end`

@paulcsmith
Copy link
Member Author

Hey @rnice01! Yeah this is a tricky one for sure! Because paths are parsed at compile time, the macro likely needs to use a constant so that it is accessible at compile time. I have not test this but I imagine it to look something like this:

module Routable
  macro included
    ROUTE_SETTINGS = {prefix: nil}
  end

  macro route_prefix(prefix)
    # Should also check that it is a `StringLiteral` and raise if it isn't
    # It should also start with a slash. See how to do that here: https://github.com/luckyframework/lucky/blob/cb207039388c2289fcb9408b92ce18a9c9825d4c/src/lucky/routable.cr#L47-L49
    {% ROUTE_SETTINGS[:prefix] = prefix %}
  end

  macro add_route(method, path, action)
    Lucky::Router.add({{ method }}, {{ ROUTE_SETTINGS[:prefix] }}{{ path }}, {{ @type.name.id }})
    # The other stuff
  end
end

Does that help?

@rnice01
Copy link
Contributor

rnice01 commented Apr 25, 2020

@paulcsmith Yes that was very helpful, thank you! I did however notice that other actions were getting prefixed as well. It appears as though the problem comes from the constant not being overridden by subclasses. This behavior appears to be intentional as noted in this issue.

I tried using the syntax suggested in the thread and was having some difficulty getting it to compile. I'll work on it some more this weekend to see if I can get it working properly.

@paulcsmith
Copy link
Member Author

@rnice01 Oh that is a great point If you look in other spots of the codebase you can see how we "inherited" constants by coping them.

macro included
EXPOSURES = [] of Symbol
macro inherited
EXPOSURES = [] of Symbol
inherit_exposures
end
end
# :nodoc:
macro inherit_exposures
\{% for v in @type.ancestors.first.constant :EXPOSURES %}
\{% EXPOSURES << v %}
\{% end %}
end

Maybe that helps! And feel free to post back here or in Gitter if you wanna chat. This stuff is triiicky!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A new requested feature / option
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants