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

Typed parameters in routes #67

Closed
vladvelici opened this issue Dec 1, 2015 · 15 comments
Closed

Typed parameters in routes #67

vladvelici opened this issue Dec 1, 2015 · 15 comments

Comments

@vladvelici
Copy link

As mentioned in expressjs/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 afebe50, 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
Copy link
Member

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
Copy link
Author

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 [email protected] 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
Copy link
Member

@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
Copy link
Author

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 [email protected] 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
Copy link
Member

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
Copy link

Perhaps this is something best left up to json schema?

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

@blakeembrey
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link
Member

👎 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
Copy link
Author

@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
Copy link
Member

@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 pillarjs/router#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
Copy link
Member

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

@blakeembrey
Copy link
Member

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
Copy link
Member

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

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

No branches or pull requests

6 participants