cerebral-legacy / cerebral-module-router

An opinionated URL change handler for Cerebral
http://cerebral-website.herokuapp.com/documentation/cerebral-module-router
19 stars 4 forks source link

Support Regex for route definitions? #42

Closed letsgetrandy closed 8 years ago

letsgetrandy commented 8 years ago

Some routes are too difficult to support by simple :param text parameters.

A regex match that passed capturing groups as parameters would be the thing that made this library one I could consider using.

Guria commented 8 years ago

Could you provide some examples how you see it?

letsgetrandy commented 8 years ago

It's a well-established pattern used in routing for frameworks in most languages, notably in the javascript world express.js and page.js.

Basically, imagine you're selling tickets, and for SEO purposes your URLs look like this:

/billy-joel-tickets/
/madonna-tickets/
/madison-square-garden-tickets/

You want one route that can handle /(.+)-tickets/, passing billy-joel or madonna or whatever into the API.

The current functionality would require a separate route defined for every possible value...

Guria commented 8 years ago

How do you suppose to define which input parameter should be passed a value?

letsgetrandy commented 8 years ago

Matching groups.

> ('/foo-bar-tickets/').match(/\/(.+)-tickets\//)
["/foo-bar-tickets/", "foo-bar"]

So in pseudocode, something like...

if (route instanceof RegExp) {
    var m = pathname.match(route);
    if (m) {
        controller.signals[signal](m.slice(1));
    } else {
        // continue on to the next route definition
} else {
    // do normal matching as string, or :param...
}
Guria commented 8 years ago

cerebral-router needs to know the name for parameter. Not only the value. Matching group has no name.

Guria commented 8 years ago

'/tickets/:artist' route triggered with '/tickets/madonna' url would call bound signal with argument:

{
  route: {
    url: '/tickets/madonna',
    path: '/tickets/madonna',
    params: { artist: 'madonna' },
    query: {}
  },
  artist: 'madonna'
}

And actions in chain would access it through input.artist. Which argument should be provided to signal in your suggestion? Note:

We can support regexps if we resolve this questions.

letsgetrandy commented 8 years ago

It's not hard to imagine ways to do it.

It could pass back {"0": "madonna"}. Or the route definition could have an optional parameter to pass in an array of property names to use, e.g. route(/\/(.+)-tickets\//, ['artist'], signal);

letsgetrandy commented 8 years ago

It's important to understand that sometimes developers don't have the luxury of redefining existing routes (which already have attached SEO value) when redesigning existing apps, or sometimes don't even have that luxury when designing new apps from scratch, due to an SEO professional on staff with specific opinions on the design of URLs.

Guria commented 8 years ago

I see a point. But I advocate technical side here. Seems with current route definition object we can't support regexp as route since regexp can't be key in object. So it very important decision and we should find a good way to resolve it.

Guria commented 8 years ago

Since we don't support defining signal more then once we could invert route definition object.

{
  'signal': '/route/with/:param',
  'signal2': {
    regexp: /\/(.+)-tickets\//,
    params: ['artist']
  }
}

cc @christianalfoni

letsgetrandy commented 8 years ago

On first glance that seems like a reasonable solution, however it does take away any ability to have more than one signal fired on a route.

Maybe routes could be defined as an array, rather than a dictionary...

[
    {
        "route": "/route/with/:param",
        "signals": [signal1, signal2]
    },
    {
        "route": {
            "regexp": /\/(.+)-tickets\//,
            "params": ['artist']
        },
        "signals": [signal3]
    }
]
Guria commented 8 years ago

We have rejected to support multiple routes for one signal since router should be able to unambiguously reconstruct url from params provided to signal. What about a "more than one signal fired on a route" is pretty much overhead. Can't find any reasons to support this. Pretty sure @christianalfoni wouldn't like this suggestion as well.

letsgetrandy commented 8 years ago

That seems kind of arbitrary to me. What exactly is the purpose of "router should be able to unambiguously reconstruct url from params"?

Isn't the job of a router to distribute incoming queries? I don't see why it would be important to go in the opposite direction.

Guria commented 8 years ago

cerebral-router just different:

The Cerebral Router as a concept allows you to create your application without thinking about routing and URLs at all. You just create signals as if you would trigger all of them manually. The router can then be hooked on and urls are routed to these already existing signals. So a URL change or manual signal trigger is transparent, it does not matter how you do it. The url will keep in sync.

letsgetrandy commented 8 years ago

I see. Well, if you guys have decided on an ideology that you feel is important to the project, feel free to ignore my feature requests. I'll just continue using other, more flexible and feature-rich libraries.

I'll leave this issue open for you to decide whether or not to close it. Thanks for the interesting discussion anyway!

Guria commented 8 years ago

It is not about flexibility. It just differs from "traditional" routing solutions. It so much win when you are just describing signals in already existing app and it works as intended.

I think regexp support would be useful and would be implemented if @christianalfoni will give green light and we decide a format to describe it.

Guria commented 8 years ago

Thank you too for your efforts, real-life examples and suggestions.

Guria commented 8 years ago

See also https://github.com/christianalfoni/addressbar#what-is-this-thing to know more about addressbar as input

christianalfoni commented 8 years ago

Hi guys and thanks for a great discussion :-)

We should definitely try to solve this. Really sorry if this comes out as overexplaining, just want to make sure we all are on the same page :-)

What we are trying to achieve with the Cerebral-Router is that:

Router(controller, {
  '/messages/:id': 'messageOpened'
})

Can be triggered by signal:

signals.messageOpened({
  id: '123'
})

or just the url /messages/123. It is the same thing. So the issue with regexp is that we need a way to trigger it manually. Making this work gives even more freedom as you are not restricted to use a manual url change or a hyperlink to change the state of the application, you can just call the signal.

So given this example:

Router(controller, {
  '/(.+)-tickets/': 'ticketsOpened'
})

We have to express this with an object, which is the payload to the signal.

signals.ticketsOpened({
  artist: 'madonna'
});

So as you say @letsgetrandy we could do this by pointing to the index of the resolved regexp. It would work, though I hope to make it even better. It would be really nice to keep the syntax of the route definition too... so this is what I suggest:

Router(controller, {
  '/(.+)-tickets/': ['artist', 'ticketsOpened']
})

As each regexp hit is an index, we could use that index in the array. Naming each regexp group. The last item in the array is the signal.

Would this make sense?

Guria commented 8 years ago

@christianalfoni how router should determine that key '/(.+)-tickets/' is regexp? By providing array as value?

christianalfoni commented 8 years ago

Good point! That is one way to do it, though we need a reverse matching when firing off the signal manually. So I would suggest that we match regexp url on paranthesis. That way we know where to put the value passed when the signal is triggered manually too.

@letsgetrandy are url regexps usually defined with paranthesis? Do you see any weakness in that logic? Or any problems with such a constraint?

christianalfoni commented 8 years ago

We can also use try/catch here: http://stackoverflow.com/questions/17250815/how-to-check-if-the-input-string-is-a-valid-regular-expression

Guria commented 8 years ago

@christianalfoni another pitfall is that not every regexp could be reconstructed back to url (eg, /\/(.+)-ticket[s]?\//)

christianalfoni commented 8 years ago

Jup, true, though that would be like say: '/messages//:id//'. So up to developer handling that I think

letsgetrandy commented 8 years ago

Perhaps rather than a true RegExp, you could just use the parens to shorten the param name?

eg:

Router(controller, {
  '/(artist)-tickets/': 'ticketsOpened'
})
Guria commented 8 years ago

vote down for try/catch. /:param is also valid regexp :)

christianalfoni commented 8 years ago

Ah, that is a really great suggestion @letsgetrandy. Would it fit all regexp scenarios?

christianalfoni commented 8 years ago

Hard to say maybe ;-)

Guria commented 8 years ago

@letsgetrandy good sollution in our case

letsgetrandy commented 8 years ago

It doesn't solve all RegExp cases, but it definitely makes things more versatile.

One case that doesn't work is optional, non-capturing groups.

/madonna-tickets/
/modonna-tickets-1/

/\/(.+)-tickets(?:-\d+)\/      // would match both.
Guria commented 8 years ago

it is not regexp at all, but just extended parametrized string. I think we could support true regexps inside matching groups only. something like:

'/(artist:.+)-tickets(variant?:-(\d+))/'
// /madonna-tickets/ => ticketsOpened({ artist: 'madonna'})
// /madonna-tickets-1/ => ticketsOpened({ artist: 'madonna', variant: '1'})
letsgetrandy commented 8 years ago

The variant thing isn't quite as important, I'm sure... But that's a pretty interesting option!

I'm sure from my suggestions, you can see that I've spent a lot of time dealing with URLs, routing, and SEO. Occasionally there are or groups, occasionally there are optional groups, but those aren't such a big deal and in a pinch they can also be handled by adding a second route.

The important one, though, is the capturing groups, like my (artist)-tickets example. I see that pattern a lot.

christianalfoni commented 8 years ago

Okay, I suggest we implement:

/(:artist)-tickets

we solve most common cases and if we are in a pinch, we can extend it with @Guria suggestion:

/(:artist .+)-tickets, or something similar. Anyways, we can extend it.

So basically we create logic that will convert:

/(:artist)-tickets into /(.+)-tickets. We keep the param in an array to match with group matches. That way we can pass the value on the correct payload key.

When firing off a signal manually we just replace the passed payload key/values on the original url string.

It being a regexp url is matched on it containg (:.+) stuff :-)

Does this seem reasonable?

Guria commented 8 years ago

Ok. I think we should start with '/(:artist)-tickets/': 'ticketsOpened' variant.

christianalfoni commented 8 years ago

Btw, this is an implementation for the url-mapper project running behind cerebrla-router really. So lets put up an issue there as soon as we feel aligned on this

Guria commented 8 years ago

url-mapper does only half of deal I suppose. Restoring url currently made in cerebral-router

christianalfoni commented 8 years ago

Ah, yes, will be changes in both projects indeed

christianalfoni commented 8 years ago

There are some other issues related to Cerebral Router too, so lets put all of this into one release. Will make a release issue summarizing everything tomorrow morning :)

christianalfoni commented 8 years ago

Thanks guys! Really appreciate the suggestions and discussions!

christianalfoni commented 8 years ago

Closing this as it is part of next release issue: https://github.com/christianalfoni/cerebral-router/issues/44

Guria commented 8 years ago

good news: https://github.com/pillarjs/path-to-regexp#custom-match-parameters used in url-mapper already has support for regexps: '/:artist(\\w+)-tickets(.*)', /madonna-tickets-anything =>

{ 
  url: '/madonna-tickets-anything',
  path: '/madonna-tickets-anything',
  params: { '0': '-anything', artist: 'madonna' },
  query: {} 
}

I will add a tests both to cerebral-router and url-mapper. And seems will have to replace hardcoded regexp with path-to-regexp api call (directly or exposed through url-mapper)

Guria commented 8 years ago

I have made initial support in #45. Moving to path-to-regexp allowed to cleanup a lot of code. It is awesome. It still wip and requires some polish but it works like a charm.