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

Static matcher #3713

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/create-matcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export function createMatcher (
redirectedFrom?: Location
): Route {
const location = normalizeLocation(raw, currentRoute, false, router)
const { name } = location
const { name, path } = location

if (name) {
const record = nameMap[name]
Expand All @@ -80,12 +80,17 @@ export function createMatcher (

location.path = fillParams(record.path, location.params, `named route "${name}"`)
return _createRoute(record, location, redirectedFrom)
} else if (location.path) {
} else if (path) {
location.params = {}
const staticRecord = pathMap[path]
if (staticRecord) {
if (matchRoute(staticRecord.regex, path, location.params)) {
return _createRoute(staticRecord, location, redirectedFrom)
}
}
for (let i = 0; i < pathList.length; i++) {
const path = pathList[i]
const record = pathMap[path]
if (matchRoute(record.regex, location.path, location.params)) {
const record = pathMap[pathList[i]]
if (matchRoute(record.regex, path, location.params)) {
return _createRoute(record, location, redirectedFrom)
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/create-route-map.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ function addRouteRecord (
})
}

if (!pathMap[record.path]) {
if (!pathMap[record.path] && !pathList.some(pathItem => {
const r = pathMap[pathItem]
return r.path.indexOf('*') === -1 && record.path.match(r.regex)
})) {
pathList.push(record.path)
pathMap[record.path] = record
}
Expand Down
2 changes: 1 addition & 1 deletion test/unit/specs/create-map.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const routes = [
children: [
{
path: '',
pathToRegexpOptions: { strict: true },
component: Baz,
name: 'bar.baz'
}
Expand Down Expand Up @@ -63,7 +64,6 @@ describe('Creating Route Map', function () {
'/bar/',
'/bar',
Copy link
Member

@posva posva Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should still work. Were they removed by accident?

Copy link
Contributor Author

@iurisilvio iurisilvio Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed because the previous rule always match before, the strict flag is false by default.

I was not convinced about this behavior, so I wrote a test to validate it.

https://github.com/buserbrasil/vue-router/commit/c247c520d121b4847d0e9d88db5368885e5bd931

I think it is an implementation detail and don't change any behavior.

It is possible to maintain them in the list, but I have to think how to do it without increasing the size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My change remove static rules that will never match because you have another rule matching before. This way I can trust if it exists in path map, it should be used instead of running through the matching loop.

Copy link
Member

@posva posva Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, these need to be added back, allowing a trailing slash when strict is false is necessary. If you think this is too much, that's fine, it's better to drop these changes on v3

Copy link
Contributor Author

@iurisilvio iurisilvio Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the test to assert that the parent route is not removed from the list when child route is strict. It don't change matching behavior.

/bar was not removed because /bar/ is strict, but /bar-redirect will never be reached, so it was removed.

I'm ok if it is not merged and I'll work on v3 soon to avoid O(n) matches.

'/bar-redirect/',
'/bar-redirect',
'*'
])
})
Expand Down
177 changes: 177 additions & 0 deletions test/unit/specs/create-matcher.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,24 @@ describe('Creating Matcher', function () {
expect(matcher.match('/p/c/n').name).toBe('nested')
})

it('match default child', function () {
const component = { name: 'fake' }
const matcher = createMatcher([
{
path: '/p',
name: 'parent',
children: [
{ path: '', name: 'parent.default', component },
{ path: 'a', name: 'parent.a', component }
]
},
{ path: '*', name: 'not-found', component }
])
expect(matcher.match('/p').name).toBe('parent.default')
expect(matcher.match('/p/').name).toBe('parent.default')
expect(matcher.match('/p/a').name).toBe('parent.a')
})

it('can get all routes', function () {
const component = { name: 'fake' }
const matcher = createMatcher([])
Expand Down Expand Up @@ -151,4 +169,163 @@ describe('Creating Matcher', function () {
expect(pathForErrorRoute).toEqual('/error/')
expect(pathForNotFoundRoute).toEqual('/')
})

it('respect ordering', function () {
const matcher = createMatcher([
{
path: '/p/staticbefore',
name: 'staticbefore',
component: { name: 'staticbefore' }
},
{
path: '/p/:id',
name: 'dynamic',
component: { name: 'dynamic' }
},
{
path: '/p/staticafter',
name: 'staticafter',
component: { name: 'staticafter' }
}
])
expect(matcher.match('/p/staticbefore').name).toBe('staticbefore')
expect(matcher.match('/p/staticafter').name).toBe('dynamic')
})

it('respect ordering for full dynamic', function () {
const matcher = createMatcher([
{
path: '/before/static',
name: 'staticbefore',
component: { name: 'staticbefore' }
},
{
path: '/:foo/static',
name: 'dynamic',
component: { name: 'dynamic' }
},
{
path: '/after/static',
name: 'staticafter',
component: { name: 'staticafter' }
}
])
expect(matcher.match('/before/static').name).toBe('staticbefore')
expect(matcher.match('/after/static').name).toBe('dynamic')
})

it('respect ordering between full dynamic and first level static', function () {
const matcher = createMatcher([
{
path: '/before/:foo',
name: 'staticbefore',
component: { name: 'staticbefore' }
},
{
path: '/:foo/static',
name: 'dynamic',
component: { name: 'dynamic' }
},
{
path: '/after/:foo',
name: 'staticafter',
component: { name: 'staticafter' }
}
])
expect(matcher.match('/before/static').name).toBe('staticbefore')
expect(matcher.match('/after/static').name).toBe('dynamic')
})

it('static can use sensitive flag', function () {
const matcher = createMatcher([
{
path: '/p/sensitive',
name: 'sensitive',
pathToRegexpOptions: {
sensitive: true
},
component: { name: 'sensitive' }
},
{
path: '/p/insensitive',
name: 'insensitive',
component: { name: 'insensitive' }
},
{
path: '*', name: 'not-found', component: { name: 'not-found ' }
}
])

expect(matcher.match('/p/SENSITIVE').name).toBe('not-found')
expect(matcher.match('/p/INSENSITIVE').name).toBe('insensitive')
})

it('static can use strict flag', function () {
const matcher = createMatcher([
{
path: '/p/strict',
name: 'strict',
pathToRegexpOptions: {
strict: true
},
component: { name: 'strict' }
},
{
path: '/p/unstrict',
name: 'unstrict',
component: { name: 'unstrict' }
},
{
path: '*', name: 'not-found', component: { name: 'not-found ' }
}
])

expect(matcher.match('/p/strict/').name).toBe('not-found')
expect(matcher.match('/p/unstrict/').name).toBe('unstrict')
})

it('static can use end flag', function () {
const matcher = createMatcher([
{
path: '/p/end',
name: 'end',
component: { name: 'end' }
},
{
path: '/p/not-end',
name: 'not-end',
pathToRegexpOptions: {
end: false
},
component: { name: 'not-end' }
},
{
path: '*', name: 'not-found', component: { name: 'not-found ' }
}
])

expect(matcher.match('/p/end/foo').name).toBe('not-found')
expect(matcher.match('/p/not-end/foo').name).toBe('not-end')
})

it('first level dynamic must work', function () {
const matcher = createMatcher([
{
path: '/:foo/b',
name: 'b',
component: { name: 'b' }
},
{
path: '/p/c',
name: 'c',
component: { name: 'c' }
},
{
path: '*', name: 'not-found', component: { name: 'not-found ' }
}
])

expect(matcher.match('/p/b').name).toBe('b')
expect(matcher.match('/p/c').name).toBe('c')
})
})