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

More routing DSL features #1292

Closed
jamesplease opened this issue Mar 17, 2017 · 6 comments
Closed

More routing DSL features #1292

jamesplease opened this issue Mar 17, 2017 · 6 comments

Comments

@jamesplease
Copy link

jamesplease commented Mar 17, 2017

Routing DSLs great: although they don't offer additional functionality over RegExp, they allow devs to write more expressive code for route matching. It's no surprise that nearly every popular routing lib on the server and client has some form of routing DSL for this reason.

Restify's routing stands out among other routers in that its routing DSL is pretty limited compared to most other routers' DSLs. Because of this, for many routes when writing Restify I'll need to write RegExps, even though my use cases are supported by the DSLs in many other routers.

It'd be cool to see some more/all of these features:

  • optional params
  • zero or more params
  • one or more params
  • custom match params
  • match-anything asterisk

These would be given for free if restify switched to path-to-regexp, which is what Express/React Router uses. Or you could write your own, like the system found in Ember's route-recognizer, or Angular's UI Router.

Are these features absolutely necessary? Nah, since RegExp does the same stuff. It just seems to be a cleaner public API to add more featurez to the DSL. I would also expect that nowadays most JS devs are more accustomed to having most of those features above in their routing DSL.

@mindfulmike
Copy link

👍

@DonutEspresso
Copy link
Member

Thanks for the feedback! The restify router is definitely one of the components that needs some TLC. I'll have to look into this in more detail, but if it is as straightforward as it seems we might be able to sneak this into the next 5.x beta.

@jamesplease
Copy link
Author

Thanks @DonutEspresso ! Would you like for me to PR a proof of concept?

@retrohacker
Copy link
Member

Hey @jmeas!

We would love to see a PR! Regexps can get pretty dense, having something that is more expressive would be a huge win.

Personally, I'd like for the changes introduced to remain backwards compatible, which doesn't sound like it would be a problem as long as we continue to support regular expressions. There are some undocumented features out there though that users may be relying on: https://github.com/restify/node-restify/pull/1065/files

One of the requirements for merging a PR is test coverage for the additions/changes, though it sounds like you have a few use cases in mind that would translate pretty well to tests ❤️

@retrohacker
Copy link
Member

For reference, this is what we are currently doing: https://github.com/restify/node-restify/blob/5.x/lib/router.js#L101-L129

I'm strongly 👍 for seeing @jmeas proposal make it into a major release, while it might break some backwards compatibility, it will be more maintainable moving forward.

@retrohacker
Copy link
Member

Migrated this to the FEATURE_REQUESTS.md file ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants