Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

Commit

Permalink
Fix handling of optionals in the middle
Browse files Browse the repository at this point in the history
Thanks so much for this library! It’s come in very handy in Shields. We’ve significantly reduced the duplication in our route definitions, and we’re getting really close letting developers build badge URLs in the frontend.

We’re running into an issue when an optional token precedes the extension. Many of the routes look something like this:

`/travis/:user/:repo/:branch?.ext(svg|png)`

- That pattern matches `/travis/foo/bar/master.svg` as we’d expect
- Unexpectedly it _does not_ match `/travis/foo/bar.svg`
- Unexpectedly it _does_ match `/travis/foo/bar/.svg`

I traced the behavior back to pillarjs#71 where it was introduced. I _think_ it was unintentional since that PR was about making the leading delimiter optional for an optional token. I don’t see why the delimiter should be present before an optional in the middle.

It’s kind of an odd fix, though it makes some sense given pillarjs#71 was about fixing leading optionals.
  • Loading branch information
paulmelnikow committed Dec 5, 2018
1 parent c36498d commit 9e7aca4
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 2 deletions.
4 changes: 2 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ function tokensToFunction (tokens) {

if (token.optional) {
// Prepend partial segment prefixes.
if (token.partial) path += token.prefix
if (i === 0 && token.partial) path += token.prefix

continue
}
Expand Down Expand Up @@ -323,7 +323,7 @@ function tokensToRegExp (tokens, keys, options) {
if (keys) keys.push(token)

if (token.optional) {
if (token.partial) {
if (i === 0 && token.partial) {
route += escapeString(token.prefix) + '(' + capture + ')?'
} else {
route += '(?:' + escapeString(token.prefix) + '(' + capture + '))?'
Expand Down
34 changes: 34 additions & 0 deletions test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,40 @@ var TESTS: Test[] = [
[{ test: 'foo' }, '/foo-bar']
]
],
[
'/:required/:optional?-ext',
null,
[
{
name: 'required',
prefix: '/',
delimiter: '/',
optional: false,
repeat: false,
partial: false,
pattern: '[^\\/]+?'
},
{
name: 'optional',
prefix: '/',
delimiter: '/',
optional: true,
repeat: false,
partial: true,
pattern: '[^\\/]+?'
},
'-ext'
],
[
['/foo-ext', ['/foo-ext', 'foo', undefined]],
['/foo/bar-ext', ['/foo/bar-ext', 'foo', 'bar']],
['/foo/-ext', null]
],
[
[{ required: 'foo' }, '/foo-ext'],
[{ required: 'foo', optional: 'baz' }, '/foo/baz-ext']
]
],
[
'/:test*-bar',
null,
Expand Down

0 comments on commit 9e7aca4

Please sign in to comment.