troch / route-node

A package to create a tree of named routes
MIT License
20 stars 15 forks source link

Path not matched if it partially matches a static route #6

Closed dogoku closed 7 years ago

dogoku commented 8 years ago

I came across an issue where a path is not correctly matched given the following routes

var routes = [
    { name: 'home', path: '/home'},
    { name: 'first', path: '/:first'}
];

matchPath('/homefoo') returns null instead of matching the route named first

I tried to debug it and it seems to be partially matching the home route and after some additional checks, it decides to return null instead of attempting to match a different route (namely first).

I'm not sure if this is expected behavior or if it's a valid bug.

Note that :

troch commented 8 years ago

Thanks for raising. Routes are re-ordered by RouteNode to go through them in a logical order, I thought that cas was handled, I'll look into it.

troch commented 8 years ago

It's in fact a known limitation. /home will be matched against your path before /:first, partially matching /homefoo. Matching follows nodes of a tree, and for performance / complexity reasons always follows the first match.

I'm not really sure how to fix this. One possibility would be to go through all siblings until a full match is found, otherwise follow the first partial match. In your case it would only work if /:first has no children. I would hide that feature behind a flag, larger projects can have dozens of routes at the top level, and it could slow down matching on the server-side when used universally.

dogoku commented 8 years ago

Hmm, I'm not sure I understand the details, but what seems strange to me is:

I did some more testing after your comment and this behavior is actually more severe than I expected, since any path that starts with /home will be be marked as null, even it has a 2nd and 3rd slug

E.g: given an extra route of { name: 'first.second', path: '/:second'}, the path /home/foo will still not match, unless I specify a { name: 'home.second', path: '/:second'} as well

let x = new RouteNode('', '', [
    new RouteNode('home', '/home'),
    new RouteNode('first', '/:first'),
    new RouteNode('first.second', '/:second')
]);
x.matchPath('/home/bar') // null
x.matchPath('/home-foo/bar') // null

x = new RouteNode('', '', [
    new RouteNode('home', '/home'),
    new RouteNode('home.second', '/:second'),
    new RouteNode('first', '/:first'),
    new RouteNode('first.second', '/:second')
]);
x.matchPath('/home/bar') // Object {name: "home.second", params: Object, _meta: Object}
x.matchPath('/home-foo/bar') // null

It seems that using static routes with the current implementation is actually pretty dangerous, especially when most of your routes can be dynamic.

troch commented 8 years ago

why try to partial match a route that has no children, instead of a full match

That is a good point, and I will try it.

why try to partial match a route that has no children, instead of a full match

/ or ? are no the only delimiters people can use in paths. For the vast majority of cases, people will break routes in segments, but I cannot make the assumption it is the case.

dogoku commented 8 years ago

I'm actually working in a branch, trying to implement my idea, but I'm still having issues

If you wanna take a look here's the commit https://github.com/dogoku/route-node/commit/3f872a7a953e633dffcdfb70d1faa1da11d106c8

Here's the gist of the changes to getSegmentsMatchingPath's for loop

const child = nodes[i];

const p = child.parser;
const children = child.getNonAbsoluteChildren();
const hasNoChildren = children.length === 0;
const hasParams = p.hasUrlParams || p.hasSpatParam || p.hasMatrixParams || p.hasQueryParams;
const singleSegmentPath = pathSegment.lastIndexOf('/') <= 0;
const segmentHasNoParams = pathSegment.indexOf('?') === -1;
const canShortCircuit =  !hasParams && segmentHasNoParams && (hasNoChildren || singleSegmentPath);

if (canShortCircuit) {
    if (child.parser.match(pathSegment, trailingSlash)) {
        segments.push(child);
        return segments;
    }
    continue;
}

// Existing logic

I'm having a problem to shortcircuit /home/:param against /homefoo/bar, since the hasNoChildren and singleSegmentPath are both false, even though it's not an exact match.

I think after messing around with the boolean operators, I managed to get it to work, but then the last assertion in the splat tests was failing. (And to be honest, I don't understand why that test is valid)

I went a bit insane last night trying to solve this puzzle. Maybe you can see something I dont

troch commented 8 years ago

OK, so I'm making changes to path-parser: partialMatch will be able to match against a delimiter (?, /, ;, possibly more). That will fix the issue.

troch commented 8 years ago

I went a bit insane last night trying to solve this puzzle. Maybe you can see something I dont

Yeah, you couldn't really achieve it just working on route-node.

I've released route-node@1.6.0. router5 is not yet updated but will be very soon.