Add .match(route) to get a matching route without invoking the handler #64
Conversation
2 similar comments
2 similar comments
|
Coveralls, you're drunk. You better also go to sleep. 👋 |
There was a problem hiding this comment.
This PR is pretty rad! - just a small comment to prevent some GC action, but otherwise this is ace!
I was thinking we should perhaps also create an issue to rewrite this module to use prototypes, and then only expose the .match() function. That way we can shave some bytes from our bundle.
index.js
Outdated
|
|
||
| throw new Error("route '" + route + "' did not match") | ||
| } | ||
|
|
||
| function nodeToObject (matched) { | ||
| return {cb: matched.cb, route: matched.cb.route, params: matched.params} |
There was a problem hiding this comment.
heya, could we perhaps statically allocate the object ahead of time, and then reuse it on later runs? The router here might be called up to 60 times a second (sure hope that it's optimized tho, haha) - but that might be problematic later down the line.
so something along the lines of:
var obj = {}
function nodeToObject (matched) {
obj.cb = matched.cb,
obj.route = matched.cb.route,
obj.params = matched.params
return obj
}There was a problem hiding this comment.
The object creation isn't an issue at all. Chrome supports millions of operations per second. Stuff like iterating through keys and assigning them like it's done in the trie file is much slower. So I'd rather remove that one https://github.com/yoshuawuyts/wayfarer/blob/master/trie.js#L99
I don't like modifying an existing object. That will cause issues. Especially because I'd also like to pass that object around and still access the correct values in a next tick.
As alternative, thi should be much faster.
function Node (matched) {
this.cb = matched.cb,
this.route = matched.cb.route,
this.params = matched.params
}There was a problem hiding this comment.
Alright, yeah I'd be cool with that!
1 similar comment
|
@yoshuawuyts updated. Thanks for the review. |
|
Thanks 🎉 Ah, I forgot that the merge & rebase button now appends the pr link to the commit. |
|
@marcbachmann ahahaha, no worries! :P |
|
Published as |
Let's begin at the bottom 😁
(see choojs/choo#613 and choojs/nanorouter#9 for more details)
This PR exposes a new
.matchmethod on the router which can be used to return a matching route without invoking it directly.I added that description to the readme:
matchedRoute = router.match(route)
Matches a route and returns an object. The returned object contains the properties
{cb, params, route}. This method does not invoke the callback of a route. If no route matches, the default route will be returned. If no default route matches, an error will be thrown.Now that
.emitcalls.match, I'm not sure how we should handle the tests.Maybe those two tests are good enough for you.
If the trie module would be private and not documented, I'd also remove the
xtendwhen returning the node as we're creating a new object anyways.