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

Route Globbing #3

Closed
russ opened this issue Dec 1, 2017 · 11 comments · Fixed by #40
Closed

Route Globbing #3

russ opened this issue Dec 1, 2017 · 11 comments · Fixed by #40

Comments

@russ
Copy link

russ commented Dec 1, 2017

I have a route in Rails that uses globbing and an optional parameter. Would be great if this router would support these features. The sample route I have looks like this:

get '/track/:code(/*path)' => 'track#track'
@paulcsmith
Copy link
Member

@russ can you provide a bit more info about how you use that route? What parameters you use, what the controller looks like, etc. I’m wondering if there are ways we can do it without a glob. And even if a glob is required it would be helpful to know the use case a bit better. Thanks so much for your input!

@russ
Copy link
Author

russ commented Dec 1, 2017

We use it for a marketing tracking setup.

Most of the time the link is just /track/:code but people can use it for deep linking by passing in /track/:code/rest/of/path to get redirected to a specific location. Being able to capture that glob in a variable lets me do the redirect accordingly.

@paulcsmith
Copy link
Member

Ah ok. That makes a lot of sense. If you were writing this from scratch, could it be done with a query param instead? Just trying to figure out possible options

@russ
Copy link
Author

russ commented Dec 1, 2017

For sure if it was from scratch but I'm dealing with over a decade of legacy links at this point. :) I know this sort of feature can really make the routing logic complex.

@paulcsmith
Copy link
Member

Ok that’s good to know. I ask because I try to prioritize things based on whether there is some kind of workaround or not. If it works for new apps, but not old ones that sucks but is better than not working ever.

I do want to make it so people can convert legacy apps to crystal though. I’ll keep this open. I have some ideas that might work while still maintaining speed and a clean code base

@russ
Copy link
Author

russ commented Dec 1, 2017

Sounds good. I'll keep an eye on this. Really liking the feel of Lucky so far.

@paulcsmith
Copy link
Member

I’m happy to hear that! Anything in particular that you like, or anything you’ve had trouble with? Just asking because it’s helpful for guiding the direction of Lucky and for figuring out what to highlight in blogs :)

@russ
Copy link
Author

russ commented Dec 1, 2017

Nothing major yet. I noticed the guides were missing some stuff around extra settings around cookies. I plan on submitting a pull request for that soon.

As far as stuff I like it's just that thought that has gone into the setup. From the initial setup, the lucky -h output being clean, and the routes table printing nicely. It's really these small sorts of things that really give it polish. Keep up the good work. I hope to be able to contribute a bit as I get time.

@paulcsmith
Copy link
Member

Thanks for the feedback! Polishing the experience is one of those unspoken goals I have for Lucky. I'm glad you noticed and enjoyed it. Feel free to hop in to https://gitter.im/luckyframework/Lobby if you have any questions or just want to chat

@robacarp
Copy link

The amber-router has this, and it wasn't a bad problem to solve. I haven't looked deeply at the way you're matching here, but the amber-router switches to attempt tail-first matching to accomplish glob routes, which might be a usable pattern here.

@paulcsmith
Copy link
Member

@robacarp That sounds awesome. As long as performance is good, then this sounds like a nice idea!

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 a pull request may close this issue.

3 participants