Hanlde multi-parametric route (issue #17) #27
Conversation
|
Can you verify if it impacts the current benchmarks anyhow? |
|
@gajus can you please check if this would work for you? |
|
This would fix the last part of fastify/fastify#122 (comment). |
index.js
Outdated
| @@ -34,6 +34,8 @@ function Router (opts) { | |||
| this.tree = new Node() | |||
| } | |||
|
|
|||
| const forbiddenCharactersInParameterName = ['-'] | |||
There was a problem hiding this comment.
This is a bit naive implementation. Rather it should accept regex for each parameter.
There was a problem hiding this comment.
Can you make (probably again) some examples of regexps that we should support?
How would you implement the check?
There was a problem hiding this comment.
There is already a complete implementation https://www.npmjs.com/package/path-to-regexp
There was a problem hiding this comment.
Rather it should accept regex for each parameter.
Ops, I didn't thought about that. I'll try to extend the current solution in the next days.
|
Hi @mcollina , |
1 similar comment
|
The latest commits should extend the support to multi parametric route. |
|
I added also a couple of cases to bench.js. |
|
It seems there is a regression also on the basic ones (static, dynamic), can you check again? The goal is for the "multi-parametric" to not slow things down for the rest. It's still a valid usecase, and we need to support it - it would be very hard to make it fast. I know that the Express router offers way more features, however it is up to 2 order of magnitude slower than this one. As long as the port from path-to-regexp to this one can be done, I'm ok. |
|
I've repeated the run a couple of times... Results are at the bottom. if (kind === 4) {
...
}Benchmark results. |
Sure! |
README.md
Outdated
| <a name="supported-path-formats"></a> | ||
| ##### Supported path formats | ||
| To register a **parametric** path, use the *colon* before the parameter name. For **wildcard** use the *star*. | ||
| *Remember that static routes should always be inserted before parametric and wildcard.* |
There was a problem hiding this comment.
This is not correct, static route should not be inserted before parametric and wildcard, they are.
We do that internally :)
There was a problem hiding this comment.
Oh, sorry for that, you're right...
I haven't done to much attention to that part before (https://github.com/delvedor/find-my-way/blob/master/node.js#L22-L30).
README.md
Outdated
| // the store can be updated | ||
| assert.equal(store, { hello: 'world' }) | ||
| }, { hello: 'world' }) | ||
| #### on(methods[], path, handler, [store]) |
There was a problem hiding this comment.
Since this is the same api I'll add another # before the section title.
README.md
Outdated
| // your code | ||
| }) | ||
| ``` | ||
| Last argument, `store` is used to pass an object that you can access later inside the handler function. |
There was a problem hiding this comment.
... the handler function. If needed, store can be updated.
Hanlde multi-parametric route (issue #17) Detect multi parametric path (new nodeType=4) Handle lookup for multi parametric path Unit test issue #17 Extend comments for new node type Detect multi parametric part with regex Hanlde lookup for multi parametric path with regex Add unit tests Extended benchmark Added another testcase Update documentation: Added info for multiparametric routes Fix documentation
4 similar comments
|
Landed in |
|
This created a problem with routes, where a param is, for example a uuid v4:
to do this now I have to use route pseudo code: Maybe I'm doing something wrong, but it does seem like a breaking change Edit:
|
Detect multi parametric path (new nodeType=4)
Handle lookup for multi parametric path
Unit test issue #17