From ce90c0ac582cefba2315838156593d5d47260d81 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Sun, 12 Oct 2014 23:05:06 +0100 Subject: [PATCH 1/6] Added support for building URLs with optional parameters Fixes some of the issues mentioned in #290 --- modules/utils/Path.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/modules/utils/Path.js b/modules/utils/Path.js index 9fb814e222..8ba1e6c8a4 100644 --- a/modules/utils/Path.js +++ b/modules/utils/Path.js @@ -15,7 +15,7 @@ function encodeURLPath(path) { } var paramCompileMatcher = /:([a-zA-Z_$][a-zA-Z0-9_$]*)|[*.()\[\]\\+|{}^$]/g; -var paramInjectMatcher = /:([a-zA-Z_$][a-zA-Z0-9_$]*)|[*]/g; +var paramInjectMatcher = /:([a-zA-Z_$?][a-zA-Z0-9_$?]*)|[*]/g; var queryMatcher = /\?(.+)/; var _compiledPatterns = {}; @@ -84,12 +84,18 @@ var Path = { var splatIndex = 0; return pattern.replace(paramInjectMatcher, function (match, paramName) { + console.log(pattern, match, paramName); paramName = paramName || 'splat'; - invariant( - params[paramName] != null, - 'Missing "' + paramName + '" parameter for path "' + pattern + '"' - ); + // If param is optional dont check for existance + if (paramName.slice(-1) !== '?') { + invariant( + params[paramName] != null, + 'Missing "' + paramName + '" parameter for path "' + pattern + '"' + ); + } else { + paramName = paramName.slice(0, -1) + } var segment; if (paramName === 'splat' && Array.isArray(params[paramName])) { From 067c8bfe3c5a0384695de82494fa88fd95ea90a8 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Sun, 12 Oct 2014 23:31:53 +0100 Subject: [PATCH 2/6] Another fix + removed console logs --- modules/utils/Path.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/utils/Path.js b/modules/utils/Path.js index 8ba1e6c8a4..3af6e27843 100644 --- a/modules/utils/Path.js +++ b/modules/utils/Path.js @@ -95,6 +95,9 @@ var Path = { ); } else { paramName = paramName.slice(0, -1) + if (params[paramName] == null) { + return ''; + } } var segment; From 488b027e86287de4e9ef753feb0003e2be98d888 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Mon, 13 Oct 2014 00:00:34 +0100 Subject: [PATCH 3/6] Added tests, based on abergs tests --- modules/utils/__tests__/Path-test.js | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/modules/utils/__tests__/Path-test.js b/modules/utils/__tests__/Path-test.js index 1766012a34..6874c2b03f 100644 --- a/modules/utils/__tests__/Path-test.js +++ b/modules/utils/__tests__/Path-test.js @@ -47,6 +47,20 @@ describe('Path.extractParams', function () { }); }); + describe('and the pattern is optional', function () { + var pattern = 'comments/:id?/edit' + + describe('and the path matches with supplied param', function () { + it('returns an object with the params', function () { + expect(Path.extractParams(pattern, 'comments/123/edit')).toEqual({ id: '123' }); + }); + }); + describe('and the path matches without supplied param', function () { + it('returns an object with param set to null', function () { + expect(Path.extractParams(pattern, 'comments//edit')).toEqual({id: null}); + }); + }); + }); describe('and the path does not match', function () { it('returns null', function () { expect(Path.extractParams(pattern, 'users/123')).toBe(null); @@ -166,6 +180,18 @@ describe('Path.injectParams', function () { }); }); + describe('and a param is optional', function () { + var pattern = 'comments/:id?/edit'; + + it('returns the correct path when param is supplied', function () { + expect(Path.injectParams(pattern, {id:'123'})).toEqual('comments/123/edit'); + }); + + it('returns the correct path when param is not supplied', function () { + expect(Path.injectParams(pattern, {})).toEqual('comments//edit'); + }); + }); + describe('and all params are present', function () { it('returns the correct path', function () { expect(Path.injectParams(pattern, { id: 'abc' })).toEqual('comments/abc/edit'); From aa80e2ffe663c2b3c73827b7f49c54a253727268 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Mon, 13 Oct 2014 13:36:12 +0100 Subject: [PATCH 4/6] Fixed issue with trailing slashes --- modules/components/Routes.js | 2 +- modules/utils/Path.js | 11 ++++++++--- modules/utils/__tests__/Path-test.js | 26 ++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/modules/components/Routes.js b/modules/components/Routes.js index 0e43de90b4..24cb3e40f8 100644 --- a/modules/components/Routes.js +++ b/modules/components/Routes.js @@ -32,7 +32,7 @@ function findMatches(path, routes, defaultRoute, notFoundRoute) { if (matches != null) { var rootParams = getRootMatch(matches).params; - + params = route.props.paramNames.reduce(function (params, paramName) { params[paramName] = rootParams[paramName]; return params; diff --git a/modules/utils/Path.js b/modules/utils/Path.js index 3af6e27843..fe1a9ec0ca 100644 --- a/modules/utils/Path.js +++ b/modules/utils/Path.js @@ -16,6 +16,7 @@ function encodeURLPath(path) { var paramCompileMatcher = /:([a-zA-Z_$][a-zA-Z0-9_$]*)|[*.()\[\]\\+|{}^$]/g; var paramInjectMatcher = /:([a-zA-Z_$?][a-zA-Z0-9_$?]*)|[*]/g; +var paramInjectTrailingSlashMatcher = /\/\/\?|\/\?/g; var queryMatcher = /\?(.+)/; var _compiledPatterns = {}; @@ -83,11 +84,10 @@ var Path = { var splatIndex = 0; - return pattern.replace(paramInjectMatcher, function (match, paramName) { - console.log(pattern, match, paramName); + var url = pattern.replace(paramInjectMatcher, function (match, paramName) { paramName = paramName || 'splat'; - // If param is optional dont check for existance + // If param is optional don't check for existence if (paramName.slice(-1) !== '?') { invariant( params[paramName] != null, @@ -114,6 +114,11 @@ var Path = { return encodeURLPath(segment); }); + url = url.replace(paramInjectTrailingSlashMatcher, '/'); + + console.log(url); + + return url; }, /** diff --git a/modules/utils/__tests__/Path-test.js b/modules/utils/__tests__/Path-test.js index 6874c2b03f..482359ecbb 100644 --- a/modules/utils/__tests__/Path-test.js +++ b/modules/utils/__tests__/Path-test.js @@ -61,6 +61,20 @@ describe('Path.extractParams', function () { }); }); }); + describe('and the pattern and forward slash are optional', function () { + var pattern = 'comments/:id?/edit' + + describe('and the path matches with supplied param', function () { + it('returns an object with the params', function () { + expect(Path.extractParams(pattern, 'comments/123/edit')).toEqual({ id: '123' }); + }); + }); + describe('and the path matches without supplied param', function () { + it('returns an object with param set to null', function () { + expect(Path.extractParams(pattern, 'comments/edit')).toEqual({id: null}); + }); + }); + }); describe('and the path does not match', function () { it('returns null', function () { expect(Path.extractParams(pattern, 'users/123')).toBe(null); @@ -192,6 +206,18 @@ describe('Path.injectParams', function () { }); }); + describe('and a param and forward slash are optional', function () { + var pattern = 'comments/:id?/?edit'; + + it('returns the correct path when param is supplied', function () { + expect(Path.injectParams(pattern, {id:'123'})).toEqual('comments/123/edit'); + }); + + it('returns the correct path when param is not supplied', function () { + expect(Path.injectParams(pattern, {})).toEqual('comments/edit'); + }); + }); + describe('and all params are present', function () { it('returns the correct path', function () { expect(Path.injectParams(pattern, { id: 'abc' })).toEqual('comments/abc/edit'); From 82c7676d13428551b25974aa60a6443bc0197929 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Mon, 13 Oct 2014 13:39:38 +0100 Subject: [PATCH 5/6] Tidied tests and removed debug prints --- modules/utils/Path.js | 9 ++------- modules/utils/__tests__/Path-test.js | 2 +- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/modules/utils/Path.js b/modules/utils/Path.js index fe1a9ec0ca..1e1f3f75ff 100644 --- a/modules/utils/Path.js +++ b/modules/utils/Path.js @@ -84,7 +84,7 @@ var Path = { var splatIndex = 0; - var url = pattern.replace(paramInjectMatcher, function (match, paramName) { + return pattern.replace(paramInjectMatcher, function (match, paramName) { paramName = paramName || 'splat'; // If param is optional don't check for existence @@ -113,12 +113,7 @@ var Path = { } return encodeURLPath(segment); - }); - url = url.replace(paramInjectTrailingSlashMatcher, '/'); - - console.log(url); - - return url; + }).replace(paramInjectTrailingSlashMatcher, '/'); }, /** diff --git a/modules/utils/__tests__/Path-test.js b/modules/utils/__tests__/Path-test.js index 482359ecbb..59af3006a3 100644 --- a/modules/utils/__tests__/Path-test.js +++ b/modules/utils/__tests__/Path-test.js @@ -62,7 +62,7 @@ describe('Path.extractParams', function () { }); }); describe('and the pattern and forward slash are optional', function () { - var pattern = 'comments/:id?/edit' + var pattern = 'comments/:id?/?edit' describe('and the path matches with supplied param', function () { it('returns an object with the params', function () { From 064e7c839f566aff3b24d837b515d11025c37211 Mon Sep 17 00:00:00 2001 From: Daniel Cannon Date: Mon, 13 Oct 2014 13:47:15 +0100 Subject: [PATCH 6/6] Fixed issue with regex allowing ":va?lue" instead of just ":value?" --- modules/utils/Path.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/utils/Path.js b/modules/utils/Path.js index 1e1f3f75ff..7d30b50da2 100644 --- a/modules/utils/Path.js +++ b/modules/utils/Path.js @@ -15,7 +15,7 @@ function encodeURLPath(path) { } var paramCompileMatcher = /:([a-zA-Z_$][a-zA-Z0-9_$]*)|[*.()\[\]\\+|{}^$]/g; -var paramInjectMatcher = /:([a-zA-Z_$?][a-zA-Z0-9_$?]*)|[*]/g; +var paramInjectMatcher = /:([a-zA-Z_$][a-zA-Z0-9_$?]*[?]?)|[*]/g; var paramInjectTrailingSlashMatcher = /\/\/\?|\/\?/g; var queryMatcher = /\?(.+)/;