Remove 'route' and 'nested_route' macro and add route inference to the generator

[paulcsmith]

Current implementation is tricky to understand and document

The current implementation (route/nested_route causes a lot of confusion because you can't see the actual route that was generated without running lucky routes, you can't easily search for the route in your codebase, and you have to look it up in the docs to understand what is happening.

However, route/nested_route is nice for enforcing sensible defaults, and for productivity

Move inference to generators

I think the way to get the best of all worlds is to build the inference in to the generator.

Examples:

*. lucky gen.action.browser Users::Show -> get "/users/:user_id"

• lucky gen.action.browser --nested Admin::Projects::Tasks::Show -> get "/admin/projects/:project_id/users/:user_id"

If you don't use a RESTful resource (like Show, Index, Delete, etc.) you will need to pass the method and urls lucky gen.action.browser Users::Something get /users/:user_id/something

• lucky gen.action.browser Home::Index GET /something-custom

[jwoertink]

I think this will be a great use to test out adding the examples to the generator. Once you start adding several arguments to a task, knowing the order, and how they should be formatted can get confusing.

Things I like about this

• Removes confusion for what is being generated
• Follows routing conventions that everyone else is already doing
• Consistency since not all action can use route or nested_route
• The generator would write these out for us

Things I will miss

• It cleans up larger code bases with some of the paths

I think it's a decent trade-off.

[hibachrach]

I'm a bit on the fence for this as I tend not to use generators in general (except when required like when initializing projects or creating new migrations)...it also does less to discourage less RESTful routes as it's not baked into the syntax.

If the issue is the additional confusion, one could add a comment above the route/nested_route invocation in the generated action file, but I suppose if you're going to do that, you might as well make it self-documenting like you're suggesting here 😛

In summary, it's probably the right move if lots of people are getting confused but I'm a bit weary regardless

[testbird]

Maybe, best of both worlds could be if the generator sets the paths explicitly together with the request type (standard thing like everbody else).
While hand typing users can still use the route or action shorthand, or instead maybe a better readable name_infered_route. (Could it even infer when to do nested-route?)

[testbird]

not much typing and makes a lot of sense just from looking at it.

Yes, that's convicing. In the end it's better to have the route written down explicitly.

[jkthorne]

I think route and nested_route are extremely helpful and help the speed of development. I like lucky but the time investment to do something simple can be way higher investment compared to rails. I think some of these APIs where inference and speed of dev is a benefit. Also the generators could use some more features just because I have leaned on them a lot to do basic things and they dont always work out of the box. I think adding a nested recourse is the best example.

[paulcsmith]

@wontruefree i think if we add route generation to the generators the speed will be the same as using route/nested_route but I’d love to hear more about what you think.

What else is slow about Lucky? I’d love to make Lucky a great dev experience for prototyping. I feel that it is but since I wrote it I understand some of the trickier parts and so it is hard to see what is stumping people.

Nested resources are definitely tricky and I think they’re a bit tricky in any framework at first. I can’t think of a way to do a generator for nested resources since it would need to know if the parent is already created or not and a bunch of other stuff. So I wonder if we just need to document it better with full examples?

[paulcsmith]

@HarrisonB i just saw your message. I like the idea of commenting. I’ll have to think about that. I think that another issue is with requiring a module namespace for nested routes. It adds a decent amount of typing and confusion for new people so not sure how to address that. I’ll try to think about it more. Maybe there is a middle ground...

[paulcsmith]

Maybe nested_route(“/articles”). This way you don’t need the module namespace and I think it’s a tad bit more clear. Still not 100% sold on it but it’s an idea

[bdtomlin]

My $0.02 😄

I would like to just switch to 100% explicit routes. It seems unnecessary to have multiple ways to do this when one gives the same functionality with added documentation.

It also seems like it is more in line with crystal’s philosophy (https://github.com/crystal-lang/crystal/wiki/FAQ#why-dont-you-add-syntax-for-xyz)

I think having multiple ways to do things when one is sufficient compounded over time can make a framework harder/easier to learn and use.

I think we will all appreciate it down the road when someone asks “how do you make an action/route” and there is one simple answer.

[paulcsmith]

@bdtomlin that makes a lot of sense. I am leaning
toward explicit but I’d love to make it as quick as possible. Maybe the generators are fine. I also personally think it’s not much more typing to type the paths.

I think I’m going to try this on some projects and see how it goes. Would love to hear what people think after trying it out

[bdtomlin]

@paulcsmith I currently just have one project in Lucky. I have used route and nested route everywhere. I will convert it to explicit routes and use them going forward in the name of science 😁

I will let you know my obviously biased opinion when I’m done converting. I usually only use generators for models and migrations though, so I might not be much help in that area.

[paulcsmith]

Thanks @bdtomlin. Looking forward to hearing what you think :)

@jwoertink what is your team using? Is it mostly route/nested_route?

[jwoertink]

We only use explicit routes, even on our restful actions. So we don't have any route or nested_route helpers.

[wout]

FWIW, I prefer explicit routes. It's just a bit more to write but so much clearer.

[paulcsmith]

@wout I kind of agree. I spend more time thinking about what the inferred path is and what the param name is than I would just writing out the path and reading it.

Gonna try it out a bit more first and see what others think after trying it, but so far it seems this will be a good way to go

[paulcsmith]

Also of note is that we can remove the parent resource namespace requirement for nested route. For example:

# Need the `Repositories` so `nested_route` knows what the parent is
class Repositories::Issues::Show < BrowserAction
  nested_route do
  end
end

# No longer need the unnecessary (and often confusing) namespace
class Issues::Show < BrowserAction
  get "/repositories/:repo_id/issues/:issue_id" do
  end
end

This is more in line with what you typically see in Rails and other web frameworks and removes some gotchas that can come up where you've got some actions nested and some without

EDIT: Also it is shorter to link to! redirect to: Issues::Show.with(issue.id) instead of redirect to: Repositories::Issues::Show.with(issue.id). So I think in the long run it may actually be less typing 😀

[bdtomlin]

I've converted all of my actions to explicit routes and here are my takeaways:

• When I created these actions I had the route in my head so it was very easy to use route or nested_route; however, when revisiting the routes it can definitely take some mental overhead to figure out what the route is. Explicit routes are really nice in this situation.
• There is some extra typing, but not enough to really be consequential.
• Adding the explicit route "feels" a little like duplication, but I don't think it quite qualifies as duplication in my mind. It's really a combination of documentation and reduced obfuscation.
• Explicit routes make it more obvious you can do something like /customer/:user_name which may not be obvious if you're used to using the route macros where you just end up referring to the user_name as customer_id
• The one thing I'm kind of on the fence about is forcing paths to begin with "/". If you don't use the path you get an error Error: Path must start with a slash. Example: '/"app/users"'. It makes me wonder what the point is and why we can't just accept either, or go with the shorter version without the slash at the front. This is really not a big deal, it just seemed a little odd.

Conclusion

I will be using explicit paths on my projects going forward. I really like the clarity and everything I learned through this process was a positive for explicit routes.

[paulcsmith]

Thank you for sharing your in put @bdtomlin. That was very thorough and helpful.

The slash requirement was added for consistency. We'd have to add more tests/code to add a slash if one doesn't exist, and I like the idea of having one way to do something. The reason for a slash and not no-slash is that then you'd have weird routes like this: get "" or get "users" which I think looks very strange and lacks clarity. Does that make sense and do you have any thoughts on improving that or do you think it's good as-is?

[paulcsmith]

Also one worry I have with explicit routes (but also applies to the route macro!) is that you may have small inconsistencies like get "/users" and in another spot get "/user/:user_id". This is not broken, but is a bit inconsistent. I think the benefits outweigh the costs though. And I think we could add a task for checking route consistency so it can find "similar" paths that are off by a character or 2 so you can fix them if you want. I think this could be added later even if we were to stick with route/nested_route because I think that'd be pretty nifty!

[bdtomlin]

@paulcsmith I understand on the slash. It kind of helps to serve as documentation saying "this is a route". I definitely prefer having one way to do things where possible as well. I definitely think this should be kept as is after hearing your thoughts and thinking about it a little.

I think a consistency check would be really nice, maybe it should be ran manually like lucky routes.check or something like that. I definitely would want to avoid adding some kind of noise if the inconsistency is intentional.

[paulcsmith]

@bdtomlin Yes good point! I think definitely a manual task that you could run as you-go or in CI if you want to automate it. And I think it should have a yaml file for ignoring certain routes if you meant for them to be slightly different so that you can fail CI for inconsistent routes, but have it pass by manually ignoring them in the yaml file.

[paulcsmith]

EDIT: There is now a milestone and issues created for this: https://github.com/luckyframework/lucky/milestone/5

I think we can close this discussion and I'll make an issue. Here is what I think we should do:

Generator changes

• Generators still infer the path. So lucky gen.action.browser Users::Show will generate an action and will set path to get "/users/:user_id".
• This makes it fast to generate actions just like it is now, and makes it clear how to modify the path and what the path is. Also allows for easily adding nesting since the path is written out: "/projects/:project_id/users/:user_id"
• Add -n or --nested option where you can set a parent from the generator: lucky gen.action.browser Users::Show -n projects will set path to "/projects/:project_id/users/:user_id". Just a shortcut so you don't need to do as much typing
• Allow generator non-REST actions by passing an HTTP method and path
  • lucky gen.action.browser Messages::Archive get /message/:message_id/archive

Other

• Extract shard for inference with route/nested_route. So people can slowly transition off, or can keep using inference if they want
• Change convention to not include the nested resource in the action name: Remove 'route' and 'nested_route' macro and add route inference to the generator #789 (comment)
  • This is mostly a documentation issue. This works today if you want it to

Keeping things clean and consistent

• Add compile time or runtime checks (prefer compile time if possible and performant) that will enforce either underscore or dashes (configurable)
• Add checks for route inconsistencies Remove 'route' and 'nested_route' macro and add route inference to the generator #789 (comment)

  And I think we could add a task for checking route consistency so it can find "similar" paths that are off by a character or 2 so you can fix them if you want.

• These checks should be disableable by using a method or macro in the action. Something like skip_route_checks. Sometimes you need to do something different or you have two similar actions but really should be different. This lets you do that and keep the checks disabled elsewhere

Moving forward

I plan to break these out into different issues so it isn't too overwhelming to work on.

I feel good about this, but if someone has any major concerns, LMK and we can see if we can improve this! If you like this, feel free to 👍 or comment

[jwoertink]

Yup! Let's do that.

[paulcsmith]

Just posted a new milestone with issues for switching this over. If you'd like to help, please comment before starting to make sure myself or someone else hasn't started already

Thanks for all the input!