Skip to content

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 #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 #71 was about fixing leading optionals.

@RedSparr0w was asking about paths like `/:optional?/:required.json` so
I added another test example to the test for that.
  • Loading branch information
paulmelnikow committed Dec 5, 2018
1 parent c36498d commit fd29ee3
Show file tree
Hide file tree
Showing 2 changed files with 38 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
36 changes: 36 additions & 0 deletions test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -827,9 +827,11 @@ var TESTS: Test[] = [
'/bar'
],
[
['/bar', ['/bar', undefined]],
['/foo/bar', ['/foo/bar', 'foo']]
],
[
[null, '/bar'],
[{ test: 'foo' }, '/foo/bar']
]
],
Expand Down Expand Up @@ -2291,6 +2293,40 @@ var TESTS: Test[] = [
[{ postType: 'random' }, null]
]
],
[
'/: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']
]
],

/**
* Unicode characters.
Expand Down

0 comments on commit fd29ee3

Please sign in to comment.