pillarjs / path-to-regexp

Turn a path string such as `/user/:name` into a regular expression
MIT License
8.2k stars 382 forks source link

Typed parameters in routes #67

Closed vladvelici closed 8 years ago

vladvelici commented 8 years ago

As mentioned in strongloop/express#2756, it would be nice to have

app.route('/:an_id{number}');
app.route('/:another_id{string}')

and an_id to actually be a number instead of string.

I implemented something here afebe509a1914d4c1c6462b269039301a8d5aa5b, but I would like to ask for feedback, and if this is something that is desirable.

I think the way I put the typedMatch function on the regexp object is not quite nice, so I'd like to hear some feedback.

Also I know it lacks documentation in the README but that should come when and if this is something desirable.

blakeembrey commented 8 years ago

Seems reasonable. This would be a breaking change release, but it's a nice feature. There are some caveats that need to be considered for the implementation though.

So, '/:an_id{number}' is basically /:an_id(\\d+|\\d+\\.\\d+) with type coercion. It's similar for other type expansions too.

Does type coercion introduce an unexpected issue here though, when your IDs get too big? For example, JS handles 2^53 safely, but beyond that things will start being weird (edit: then we hit Infinity).

Does that just get documented, or do we just always return strings so people can then use libraries like bigint?

Does {number} support other numeric notations that JavaScript does in the Number constructor? For instance, Number('0x10')?

vladvelici commented 8 years ago

If the numbers too big issue is addressed as always returning strings then {number} is just a nicer way to say (\d.\d|\d).

Documenting it is certainly necessary. I suggest also having a way to provide custom parsers for different types, as an optional config step after require, not necessarily as a per-route thing (but this can work as well). It can also be a way to define new types, e.g. Date.

Regarding the '0x10' situation, it probably fits in the idea of allowing configurable parsers for types.

On 1 Dec 2015, at 23:08, Blake Embrey notifications@github.com wrote:

Seems reasonable. This would be a breaking change release, but it's a nice feature. There are some caveats that need to be considered for the implementation though.

So, '/:an_id{number}' is basically /:an_id(\d+|\d+.\d+) with type coercion. It's similar for other type expansions too.

Does type coercion introduce an unexpected issue here though, when your IDs get too big? For example, JS handles 2^53 safely, but beyond that things will start being weird.

Does that just get documented, or do we just always return strings so people can then use libraries like bigint?

Does {number} support other numeric notations that JavaScript does in the Number constructor? For instance, Number('0x10')?

— Reply to this email directly or view it on GitHub.

blakeembrey commented 8 years ago

@vladvelici Once you bring in configurable parsers, however cool, this sort of leaves path-to-regexp and becomes it's own thing. My comment in Express on the custom router implementation might be a good pointer for that type of implementation.

As for numbers too big, it should either always be a string or a number. So does it coerce or not? That was mostly my concern there, and if it does parse when does it fail.

vladvelici commented 8 years ago

By configurable parsers I was thinking somethings like configurable global types, like:

pathToRegexp.types = {
    number: parseFloat,
    integer: parseInt,
    bigint: function (str) {...},
    date: ...
}

Which then can be used like

/:foo{bigint}
/:bar{number}
/:foo2{date}

But I agree, it goes a bit beyond the scope of this project.

On 1 Dec 2015, at 23:43, Blake Embrey notifications@github.com wrote:

@vladvelici Once you bring in configurable parsers, however cool, this sort of leaves path-to-regexp and becomes it's own thing. My comment in Express on the custom router implementation might be a good pointer for that type of implementation.

As for numbers too big, it should either always be a string or a number. So does it coerce or not? That was mostly my concern there, and if it does parse when does it fail.

— Reply to this email directly or view it on GitHub.

blakeembrey commented 8 years ago

The problem with that and why it's out of scope is that you'll be modifying any other usages of path-to-regexp. It would need to be an initializer or config option instead, which would mean it's not as plug-and-play as currently. That's not a problem, just a different solution.

Anyway, I understand what you want now. How would a plugin syntax like that be optimized, for instance? Numbers don't need to match strings, but here we don't understand the syntax used if the parser is a function. If we pass the function everything matched, the implementation would need to signal back that it's either valid or not (for instance, to go to the next route and/or 404 - in Express-land).

metasansana commented 8 years ago

Perhaps this is something best left up to json schema?

https://www.npmjs.com/package/jsonschema

blakeembrey commented 8 years ago

JSON schema is a whole other beast - it's a complete schema implementation is generally quite complex. Most parameters tend to be little more than types like number and string, so probably overkill here. Personally, I'd recommend middleware in your target implementation (E.g. Express) that validates the params on some kind of schema (maybe even JSON schema).

wesleytodd commented 8 years ago

IMHO, the basic route matching semantics should be as slim as possible. I think the issue @blakeembrey mentioned here is a better solution because every new feature is a performance hit. So if you need that feature I would think implementing a routing engine would be best.

fourpixels commented 8 years ago

I second what @wesleytodd says. I don't find it that hard for manually cast to whatever type you need. Having most of the available types will bring a hell of a mess (just imagine how you set Date type, not to speak of encoding).

Don't forget that url is a string, and the way you interpret it is your own job. The routing system itself does (and should) not care if this is a Number or a String.

jonathanong commented 8 years ago

:-1: could easily be a separate module that wraps this module.

i'd take a PR here: https://github.com/pillarjs/path-match but it isn't used by express so...

vladvelici commented 8 years ago

@blakeembrey Optimisation - it can be made such that each parser has a regexp and the parsing function. The syntax might not be as nice as integer: parseInt, but it can be integer: {f: parseInt, regexp: /[0-9]+/} which is not too bad. Configuration can be optional, only if you care about special cases like integers too big to fit number, or you want to add other custom objects.

@wesleytodd Agree with the fact that every new feature is a performance hit.

General: I don't particularly need this feature. I just played around and I thought it's a nice thing to have. Sometimes I find it annoying to do parseInt(...) or parseFloat(...) on parameters. I also agree that this feature would add a lot of complexity for this project which is certainly not ideal. I initially liked it because it's so small and does one thing well.

@jonathanong Seems like path-match is a better fit for this suggestion. I'll send a PR there when I get a bit of time (I assume it needs some adjustments?).

I'm closing this issue now. Thanks for the responses everybody!

blakeembrey commented 8 years ago

@vladvelici It definitely should exist. I've commented on a similar issue in Express and I thought I'd left comments here, but I actually did similar work a year ago. For example, to make this work altogether I created https://github.com/pillarjs/router/pull/29 and https://github.com/mulesoft-labs/raml-path-match.

@jonathanong Can I make the PR on path-match to standardize the output? I can make it match with raml-path-match, which would allow us to already have two new path matching semantics in Express 5.x.

Edit: Here's the router I built that has a simple type system based on an API specification language: https://github.com/mulesoft-labs/osprey-router.

jonathanong commented 8 years ago

@blakeembrey np as long as you don't break backwards compat :)

blakeembrey commented 8 years ago

It would, it'd need a major version bump. The output from path-match couldn't be used for customized routers, because in .use you need to be able to get the original path match to strip it. That's why I tweaked your matching behaviour a little bit to make it work.

jonathanong commented 8 years ago

hm not sure i follow. open a new issue? i don't really mind a breaking change though as long as there is added benefit, just prefer not to break anything