Skip to content

Commit

Permalink
fix($urlMatcherFactory): default to parameter string coersion.
Browse files Browse the repository at this point in the history 
feat($urlMatcherFactory): unify params handling code for path/search
feat($urlMatcherFactory): add a defaultType that does string coersion
and supports arrays (for params)
feat($urlMatcherFactory): separate default Type(s) for path/query params

Closes #1414
  • Loading branch information
@christopherthielen
christopherthielen committed Oct 21, 2014
1 parent 838b747 commit 13a468a
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 42 deletions.
5 changes: 2 additions & 3 deletions src/state.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,8 +784,7 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) {
if (options.inherit) toParams = inheritParams($stateParams, toParams || {}, $state.$current, toState);
if (!toState.params.$$validates(toParams)) return TransitionFailed;

var defaultParams = toState.params.$$values();
toParams = extend(defaultParams, toParams);
toParams = toState.params.$$values(toParams);
to = toState;

var toPath = to.path;
Expand All @@ -794,7 +793,7 @@ function $StateProvider( $urlRouterProvider, $urlMatcherFactory) {
var keep = 0, state = toPath[keep], locals = root.locals, toLocals = [];

if (!options.reload) {
while (state && state === fromPath[keep] && equalForKeys(toParams, fromParams, state.ownParams.$$keys())) {
while (state && state === fromPath[keep] && state.ownParams.$$equals(toParams, fromParams)) {
locals = toLocals[keep] = state.locals;
keep++;
state = toPath[keep];
Expand Down
90 changes: 64 additions & 26 deletions src/urlMatcherFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,14 @@ function UrlMatcher(pattern, config) {
// The regular expression is somewhat complicated due to the need to allow curly braces
// inside the regular expression. The placeholder regexp breaks down as follows:
// ([:*])(\w+) classic placeholder ($1 / $2)
// \{(\w+)(?:\:( ... ))?\} curly brace placeholder ($3) with optional regexp ... ($4)
// ([:]?)([\w-]+) classic search placeholder (supports snake-case-params) ($1 / $2)
// \{(\w+)(?:\:( ... ))?\} curly brace placeholder ($3) with optional regexp/type ... ($4)
// (?: ... | ... | ... )+ the regexp consists of any number of atoms, an atom being either
// [^{}\\]+ - anything other than curly braces or backslash
// \\. - a backslash escape
// \{(?:[^{}\\]+|\\.)*\} - a matched set of curly braces containing other atoms
var placeholder = /([:*])(\w+)|\{(\w+)(?:\:((?:[^{}\\]+|\\.|\{(?:[^{}\\]+|\\.)*\})+))?\}/g,
searchPlaceholder = /([:]?)([\w-]+)|\{(\w+)(?:\:((?:[^{}\\]+|\\.|\{(?:[^{}\\]+|\\.)*\})+))?\}/g,
compiled = '^', last = 0, m,
segments = this.segments = [],
params = this.params = new $$UrlMatcherFactoryProvider.ParamSet();
Expand All @@ -94,32 +96,33 @@ function UrlMatcher(pattern, config) {
return result + flag + '(' + pattern + ')' + flag;
}

function regexpType(regexp) {
var type = new Type({
pattern: new RegExp(regexp),
is: function(value) { return type.pattern.exec(type.encode(value)); }
});
return type;
}

this.source = pattern;

// Split into static segments separated by path parameter placeholders.
// The number of segments is always 1 more than the number of parameters.
var id, regexp, segment, type, cfg;

while ((m = placeholder.exec(pattern))) {
function matchDetails(m, isSearch) {
var id, regexp, segment, type, typeId, cfg;
var $types = UrlMatcher.prototype.$types;
var defaultTypeId = (isSearch ? "searchParam" : "pathParam");
id = m[2] || m[3]; // IE[78] returns '' for unmatched groups instead of null
regexp = m[4] || (m[1] == '*' ? '.*' : '[^/]*');
segment = pattern.substring(last, m.index);
type = this.$types[regexp] || regexpType(regexp);
regexp = isSearch ? m[4] : m[4] || (m[1] == '*' ? '.*' : null);
typeId = regexp || defaultTypeId;
type = $types[typeId] || extend({}, $types[defaultTypeId], { pattern: new RegExp(regexp) });
cfg = config.params[id];
return {
id: id, regexp: regexp, segment: segment, type: type, cfg: cfg
};
}

if (segment.indexOf('?') >= 0) break; // we're into the search part
var p, param, segment;
while ((m = placeholder.exec(pattern))) {
p = matchDetails(m, false);
if (p.segment.indexOf('?') >= 0) break; // we're into the search part

var param = addParameter(id, type, cfg);
compiled += quoteRegExp(segment, type.$subPattern(), param.isOptional);
segments.push(segment);
param = addParameter(p.id, p.type, p.cfg);
compiled += quoteRegExp(p.segment, param.type.pattern.source, param.isOptional);
segments.push(p.segment);
last = placeholder.lastIndex;
}
segment = pattern.substring(last);
Expand All @@ -132,10 +135,15 @@ function UrlMatcher(pattern, config) {
segment = segment.substring(0, i);
this.sourcePath = pattern.substring(0, last + i);

// Allow parameters to be separated by '?' as well as '&' to make concat() easier
forEach(search.substring(1).split(/[&?]/), function(key) {
addParameter(key, null, config.params[key]);
});
if (search.length > 0) {
last = 0;
while ((m = searchPlaceholder.exec(search))) {
p = matchDetails(m, true);
param = addParameter(p.id, p.type, p.cfg);
last = placeholder.lastIndex;
// check if ?&
}
}
} else {
this.sourcePath = pattern;
this.sourceSearch = '';
Expand Down Expand Up @@ -385,7 +393,7 @@ Type.prototype.encode = function(val, key) {
* @methodOf ui.router.util.type:Type
*
* @description
* Converts a string URL parameter value to a custom/native value.
* Converts a parameter value (from URL string or transition param) to a custom/native value.
*
* @param {string} val The URL parameter value to decode.
* @param {string} key The name of the parameter in which `val` is stored. Can be used for
Expand Down Expand Up @@ -433,7 +441,36 @@ function $UrlMatcherFactory() {

var isCaseInsensitive = false, isStrictMode = true;

function safeString(val) { return isDefined(val) ? val.toString() : val; }
function coerceEquals(left, right) { return left == right; }
function angularEquals(left, right) { return angular.equals(left, right); }
// TODO: function regexpMatches(val) { return isDefined(val) && this.pattern.test(val); }
function regexpMatches(val) { /*jshint validthis:true */ return this.pattern.test(val); }
function normalizeStringOrArray(val) {
if (isArray(val)) {
var encoded = [];
forEach(val, function(item) { encoded.push(safeString(item)); });
return encoded;
} else {
return safeString(val);
}
}

var enqueue = true, typeQueue = [], injector, defaultTypes = {
"searchParam": {
encode: normalizeStringOrArray,
decode: normalizeStringOrArray,
equals: angularEquals,
is: regexpMatches,
pattern: /[^&?]*/
},
"pathParam": {
encode: safeString,
decode: safeString,
equals: coerceEquals,
is: regexpMatches,
pattern: /[^/]*/
},
int: {
decode: function(val) {
return parseInt(val, 10);
Expand All @@ -449,7 +486,7 @@ function $UrlMatcherFactory() {
return val ? 1 : 0;
},
decode: function(val) {
return parseInt(val, 10) === 0 ? false : true;
return parseInt(val, 10) !== 0;
},
is: function(val) {
return val === true || val === false;
Expand Down Expand Up @@ -720,8 +757,9 @@ function $UrlMatcherFactory() {

function getType(config, urlType) {
if (config.type && urlType) throw new Error("Param '"+id+"' has two type configurations.");
if (urlType && !config.type) return urlType;
return config.type instanceof Type ? config.type : new Type(config.type || {});
if (urlType) return urlType;
if (!config.type) return UrlMatcher.prototype.$types.pathParam;
return config.type instanceof Type ? config.type : new Type(config.type);
}

/**
Expand Down
6 changes: 3 additions & 3 deletions test/stateDirectivesSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('uiStateRef', function() {
$q.flush();

expect($state.current.name).toEqual('contacts.item.detail');
expect($stateParams).toEqual({ id: 5 });
expect($stateParams).toEqual({ id: "5" });
}));

it('should transition when given a click that contains no data (fake-click)', inject(function($state, $stateParams, $q) {
Expand All @@ -147,7 +147,7 @@ describe('uiStateRef', function() {
$q.flush();

expect($state.current.name).toEqual('contacts.item.detail');
expect($stateParams).toEqual({ id: 5 });
expect($stateParams).toEqual({ id: "5" });
}));

it('should not transition states when ctrl-clicked', inject(function($state, $stateParams, $q) {
Expand Down Expand Up @@ -328,7 +328,7 @@ describe('uiStateRef', function() {
$q.flush();

expect($state.$current.name).toBe("contacts.item.detail");
expect($state.params).toEqual({ id: 5 });
expect($state.params).toEqual({ id: "5" });
}));

it('should resolve states from parent uiView', inject(function ($state, $stateParams, $q, $timeout) {
Expand Down
59 changes: 49 additions & 10 deletions test/stateSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ describe('state', function () {
HH = { parent: H },
HHH = {parent: HH, data: {propA: 'overriddenA', propC: 'propC'} },
RS = { url: '^/search?term', reloadOnSearch: false },
OPT = { url: '/opt/:param', params: { param: 100 } },
OPT = { url: '/opt/:param', params: { param: "100" } },
OPT2 = { url: '/opt2/:param2/:param3', params: { param3: "300", param4: "400" } },
AppInjectable = {};

beforeEach(module(function ($stateProvider, $provide) {
Expand All @@ -48,6 +49,7 @@ describe('state', function () {
.state('HH', HH)
.state('HHH', HHH)
.state('OPT', OPT)
.state('OPT.OPT2', OPT2)
.state('RS', RS)

.state('home', { url: "/" })
Expand Down Expand Up @@ -273,7 +275,7 @@ describe('state', function () {
$q.flush();
expect(called).toBeTruthy();
expect($state.current.name).toEqual('DDD');
expect($state.params).toEqual({ x: 1, y: 2, z: 3, w: 4 });
expect($state.params).toEqual({ x: "1", y: "2", z: "3", w: "4" });
}));

it('can defer a state transition in $stateNotFound', inject(function ($state, $q, $rootScope) {
Expand All @@ -290,7 +292,7 @@ describe('state', function () {
$q.flush();
expect(called).toBeTruthy();
expect($state.current.name).toEqual('AA');
expect($state.params).toEqual({ a: 1 });
expect($state.params).toEqual({ a: "1" });
}));

it('can defer and supersede a state transition in $stateNotFound', inject(function ($state, $q, $rootScope) {
Expand Down Expand Up @@ -479,11 +481,11 @@ describe('state', function () {
$state.transitionTo('about.person', { person: 'bob' });
$q.flush();

$state.go('.item', { id: 5 });
$state.go('.item', { id: "5" });
$q.flush();

expect($state.$current.name).toBe('about.person.item');
expect($stateParams).toEqual({ person: 'bob', id: 5 });
expect($stateParams).toEqual({ person: 'bob', id: "5" });

$state.go('^.^.sidebar');
$q.flush();
Expand Down Expand Up @@ -751,6 +753,7 @@ describe('state', function () {
'HH',
'HHH',
'OPT',
'OPT.OPT2',
'RS',
'about',
'about.person',
Expand Down Expand Up @@ -799,16 +802,52 @@ describe('state', function () {
$state.get("OPT").onEnter = function($stateParams) { stateParams = $stateParams; };
$state.go("OPT"); $q.flush();
expect($state.current.name).toBe("OPT");
expect($state.params).toEqual({ param: 100 });
expect(stateParams).toEqual({ param: 100 });
expect($state.params).toEqual({ param: "100" });
expect(stateParams).toEqual({ param: "100" });
}));

it("should be populated during primary transition, if unspecified", inject(function($state, $q) {
var count = 0;
$state.get("OPT").onEnter = function($stateParams) { count++; };
$state.go("OPT"); $q.flush();
expect($state.current.name).toBe("OPT");
expect($state.params).toEqual({ param: 100 });
expect($state.params).toEqual({ param: "100" });
expect(count).toEqual(1);
}));

it("should allow mixed URL and config params", inject(function($state, $q) {
var count = 0;
$state.get("OPT").onEnter = function($stateParams) { count++; };
$state.get("OPT.OPT2").onEnter = function($stateParams) { count++; };
$state.go("OPT"); $q.flush();
expect($state.current.name).toBe("OPT");
expect($state.params).toEqual({ param: "100" });
expect(count).toEqual(1);

$state.go("OPT.OPT2", { param2: 200 }); $q.flush();
expect($state.current.name).toBe("OPT.OPT2");
expect($state.params).toEqual({ param: "100", param2: "200", param3: "300", param4: "400" });
expect(count).toEqual(2);
}));
});

// TODO: Enforce by default in next major release (1.0.0)
xdescribe('non-optional parameters', function() {
it("should cause transition failure, when unspecified.", inject(function($state, $q) {
var count = 0;
$state.get("OPT").onEnter = function() { count++; };
$state.get("OPT.OPT2").onEnter = function() { count++; };
$state.go("OPT"); $q.flush();
expect($state.current.name).toBe("OPT");
expect($state.params).toEqual({ param: "100" });
expect(count).toEqual(1);

var result;
$state.go("OPT.OPT2").then(function(data) { result = data; });
$q.flush();
expect($state.current.name).toBe("OPT");
expect($state.params).toEqual({ param: "100" });
expect(result).toEqual("asdfasdf");
expect(count).toEqual(1);
}));
});
Expand Down Expand Up @@ -996,7 +1035,7 @@ describe('state', function () {
$state.go('root.sub1', { param2: 2 });
$q.flush();
expect($state.current.name).toEqual('root.sub1');
expect($stateParams).toEqual({ param1: 1, param2: 2 });
expect($stateParams).toEqual({ param1: "1", param2: "2" });
}));

it('should not inherit siblings\' states', inject(function ($state, $stateParams, $q) {
Expand All @@ -1009,7 +1048,7 @@ describe('state', function () {
$q.flush();
expect($state.current.name).toEqual('root.sub2');

expect($stateParams).toEqual({ param1: 1, param2: undefined });
expect($stateParams).toEqual({ param1: "1", param2: undefined });
}));
});

Expand Down

0 comments on commit 13a468a

Please sign in to comment.