Skip to content

Commit

Permalink
BREAKING CHANGE: Order URL Matching Rules by priority, not registrati…
Browse files Browse the repository at this point in the history
…on order

URL Rules can come from registered states' `.url`s, calling `.when()`, or calling `.rule()`.
It's possible that two or more URL Rules could match the URL.

### Previously

Previously, url rules were matched in the order in which they were registered.
The rule which was registered first would handle the URL change.

### Now

Now, the URL rules are sorted according to a sort function.
More specific rules are preferred over less specific rules

### Why

It's possible to have multiple url rules that match a given URL.
Consider the following states:

 - `{ name: 'books', url: '/books/index' }''`
 - `{ name: 'book', url: '/books/:bookId' }''`

Both states match when the url is `/books/index`.
Additionally, you might have some custom url rewrite rules such as:

 `.when('/books/list', '/books/index')`.

The `book` state also matches when the rewrite rule is matched.

Previously, we simply used the first rule that matched.  However, now that lazy loading is officially supported, it can be difficult for developers to ensure the rules are registered in the right order.

Instead, we now prioritize url rules by how specific they are.  More specific rules are matched earlier than less specific rules.
We split the path on `/`.  A static segment (such as `index` in the example) is more specific than a parameter (such as`:bookId`).

### More Details

The built-in rule sorting function (see `UrlRouter.defaultRuleSortFn`) sorts rules in this order:

- Explicit priority: `.when('/foo', '/bar', { priority: 1 })` (default priority is 0)
- Rule Type:
  - UrlMatchers first (registered states and `.when(string, ...)`)
  - then regular Expressions (`.when(regexp, ...)`)
  - finally, everything else (`.rule()`)
- UrlMatcher specificity: static path segments are more specific than variables (see `UrlMatcher.compare`)
- Registration order (except for UrlMatcher based rules)

For complete control, a custom sort function can be registered with `UrlService.rules.sort(sortFn)`

### Query params

Because query parameters are optional, they are not considered during sorting.
For example, both these rules will match when the url is `'/foo/bar'`:

```
.when('/foo/bar', doSomething);
.when('/foo/bar?queryparam', doSomethingElse);
```

To choose the most specific rule, we match both rules, then choose the rule with the "best ratio" of matched optional parameters (see `UrlRuleFactory.fromUrlMatcher`)

This allows child states to be defined with only query params for a URL.
The state only activates when the query parameter is present.

```
.state('parent', { url: '/parent' });
.state('parent.child', { url: '?queryParam' });
```

## Restoring the previous behavior

For backwards compatibility, register a sort function which sorts by the registration order:

```js
myApp.config(function ($urlServiceProvider) {

  function sortByRegistrationOrder(a, b) {
   return a.$id - b.$id;
  }

  $urlServiceProvider.rules.sort(sortByRegistrationOrder);

});
```

---

feat(UrlRouter): sort url rules by specificity, not by registration order.
refactor(UrlMatcher): Include own matcher in matcher._cache.path
feat(UrlMatcher): Add comparison function by UrlMatcher specificity
refactor(UrlRule): Use interface for UrlRules instead of extending classes
  • Loading branch information
christopherthielen committed Dec 29, 2016
1 parent 7334d98 commit eb2f5d7
Show file tree
Hide file tree
Showing 13 changed files with 852 additions and 414 deletions.
61 changes: 57 additions & 4 deletions src/common/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
* @module common
*/ /** for typedoc */

import {isFunction, isString, isArray, isRegExp, isDate} from "./predicates";
import { all, any, not, prop, curry } from "./hof";
import {services} from "./coreservices";
import {State} from "../state/stateObject";
import { isFunction, isString, isArray, isRegExp, isDate } from "./predicates";
import { all, any, prop, curry, val } from "./hof";
import { services } from "./coreservices";
import { State } from "../state/stateObject";

let w: any = typeof window === 'undefined' ? {} : window;
let angular = w.angular || {};
Expand Down Expand Up @@ -607,6 +607,59 @@ function _arraysEq(a1: any[], a2: any[]) {
return arrayTuples(a1, a2).reduce((b, t) => b && _equals(t[0], t[1]), true);
}

export type sortfn = (a,b) => number;

/**
* Create a sort function
*
* Creates a sort function which sorts by a numeric property.
*
* The `propFn` should return the property as a number which can be sorted.
*
* #### Example:
* This example returns the `priority` prop.
* ```js
* var sortfn = sortBy(obj => obj.priority)
* // equivalent to:
* var longhandSortFn = (a, b) => a.priority - b.priority;
* ```
*
* #### Example:
* This example uses [[prop]]
* ```js
* var sortfn = sortBy(prop('priority'))
* ```
*
* The `checkFn` can be used to exclude objects from sorting.
*
* #### Example:
* This example only sorts objects with type === 'FOO'
* ```js
* var sortfn = sortBy(prop('priority'), propEq('type', 'FOO'))
* ```
*
* @param propFn a function that returns the property (as a number)
* @param checkFn a predicate
*
* @return a sort function like: `(a, b) => (checkFn(a) && checkFn(b)) ? propFn(a) - propFn(b) : 0`
*/
export const sortBy = (propFn: (a) => number, checkFn: Predicate<any> = val(true)) =>
(a, b) =>
(checkFn(a) && checkFn(b)) ? propFn(a) - propFn(b) : 0;

/**
* Composes a list of sort functions
*
* Creates a sort function composed of multiple sort functions.
* Each sort function is invoked in series.
* The first sort function to return non-zero "wins".
*
* @param sortFns list of sort functions
*/
export const composeSort = (...sortFns: sortfn[]): sortfn =>
(a, b) =>
sortFns.reduce((prev, fn) => prev || fn(a, b), 0);

// issue #2676
export const silenceUncaughtInPromise = (promise: Promise<any>) =>
promise.catch(e => 0) && promise;
Expand Down
50 changes: 43 additions & 7 deletions src/common/strings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
* @module common_strings
*/ /** */

import {isString, isArray, isDefined, isNull, isPromise, isInjectable, isObject} from "./predicates";
import {Rejection} from "../transition/rejectFactory";
import {IInjectable, identity, Obj} from "./common";
import {pattern, is, not, val, invoke} from "./hof";
import {Transition} from "../transition/transition";
import {Resolvable} from "../resolve/resolvable";
import { isString, isArray, isDefined, isNull, isPromise, isInjectable, isObject } from "./predicates";
import { Rejection } from "../transition/rejectFactory";
import { IInjectable, identity, Obj, tail, pushR } from "./common";
import { pattern, is, not, val, invoke } from "./hof";
import { Transition } from "../transition/transition";
import { Resolvable } from "../resolve/resolvable";

/**
* Returns a string shortened to a maximum length
Expand Down Expand Up @@ -116,4 +116,40 @@ export const beforeAfterSubstr = (char: string) => (str: string) => {
let idx = str.indexOf(char);
if (idx === -1) return [str, ""];
return [str.substr(0, idx), str.substr(idx + 1)];
};
};

/**
* Splits on a delimiter, but returns the delimiters in the array
*
* #### Example:
* ```js
* var splitOnSlashes = splitOnDelim('/');
* splitOnSlashes("/foo"); // ["/", "foo"]
* splitOnSlashes("/foo/"); // ["/", "foo", "/"]
* ```
*/
export function splitOnDelim(delim: string) {
let re = new RegExp("(" + delim + ")", "g");
return (str: string) =>
str.split(re).filter(identity);
};


/**
* Reduce fn that joins neighboring strings
*
* Given an array of strings, returns a new array
* where all neighboring strings have been joined.
*
* #### Example:
* ```js
* let arr = ["foo", "bar", 1, "baz", "", "qux" ];
* arr.reduce(joinNeighborsR, []) // ["foobar", 1, "bazqux" ]
* ```
*/
export function joinNeighborsR(acc: any[], x: any) {
if (isString(tail(acc)) && isString(x))
return acc.slice(0, -1).concat(tail(acc)+ x);
return pushR(acc, x);
};

26 changes: 15 additions & 11 deletions src/state/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,15 @@
* @coreapi
* @module state
*/ /** for typedoc */
import { ParamDeclaration, RawParams } from "../params/interface";

import {State} from "./stateObject";
import {ViewContext} from "../view/interface";
import {IInjectable} from "../common/common";
import {Transition} from "../transition/transition";
import {TransitionStateHookFn} from "../transition/interface";
import {ResolvePolicy, ResolvableLiteral} from "../resolve/interface";
import {Resolvable} from "../resolve/resolvable";
import {ProviderLike} from "../resolve/interface";
import {TargetState} from "./targetState";
import { ParamDeclaration, RawParams, ParamsOrArray } from "../params/interface";
import { State } from "./stateObject";
import { ViewContext } from "../view/interface";
import { IInjectable } from "../common/common";
import { Transition } from "../transition/transition";
import { TransitionStateHookFn, TransitionOptions } from "../transition/interface";
import { ResolvePolicy, ResolvableLiteral, ProviderLike } from "../resolve/interface";
import { Resolvable } from "../resolve/resolvable";
import { TargetState } from "./targetState";

export type StateOrName = (string|StateDeclaration|State);

Expand All @@ -21,6 +19,12 @@ export interface TransitionPromise extends Promise<State> {
transition: Transition;
}

export interface TargetStateDef {
state: StateOrName;
params?: ParamsOrArray;
options?: TransitionOptions;
}

export type ResolveTypes = Resolvable | ResolvableLiteral | ProviderLike;
/**
* Base interface for [[Ng1ViewDeclaration]] and [[Ng2ViewDeclaration]]
Expand Down
3 changes: 0 additions & 3 deletions src/state/stateObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ export class State {
/** A compiled URLMatcher which detects when the state's URL is matched */
public url: UrlMatcher;

/** @hidden temporary place to put the rule registered with $urlRouter.when() */
public _urlRule: any;

/** The parameters for the state, built from the URL and [[StateDefinition.params]] */
public params: { [key: string]: Param };

Expand Down
3 changes: 1 addition & 2 deletions src/state/stateQueueManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ export class StateQueueManager implements Disposable {
attachRoute(state: State) {
if (state.abstract || !state.url) return;

state._urlRule = this.$urlRouter.urlRuleFactory.fromState(state);
this.$urlRouter.addRule(state._urlRule);
this.$urlRouter.rule(this.$urlRouter.urlRuleFactory.create(state));
}
}
11 changes: 6 additions & 5 deletions src/state/stateRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { StateQueueManager } from "./stateQueueManager";
import { StateDeclaration } from "./interface";
import { BuilderFunction } from "./stateBuilder";
import { StateOrName } from "./interface";
import { UrlRouter } from "../url/urlRouter";
import { removeFrom } from "../common/common";
import { UIRouter } from "../router";
import { propEq } from "../common/hof";

/**
* The signature for the callback function provided to [[StateRegistry.onStateRegistryEvent]].
Expand All @@ -31,13 +31,11 @@ export class StateRegistry {
matcher: StateMatcher;
private builder: StateBuilder;
stateQueue: StateQueueManager;
urlRouter: UrlRouter;

listeners: StateRegistryListener[] = [];

/** @internalapi */
constructor(private _router: UIRouter) {
this.urlRouter = _router.urlRouter;
this.matcher = new StateMatcher(this.states);
this.builder = new StateBuilder(this.matcher, _router.urlMatcherFactory);
this.stateQueue = new StateQueueManager(this, _router.urlRouter, this.states, this.builder, this.listeners);
Expand Down Expand Up @@ -143,10 +141,13 @@ export class StateRegistry {
};

let children = getChildren([state]);
let deregistered = [state].concat(children).reverse();
let deregistered: State[] = [state].concat(children).reverse();

deregistered.forEach(state => {
this.urlRouter.removeRule(state._urlRule);
let $ur = this._router.urlRouter;
// Remove URL rule
$ur.rules().filter(propEq("state", state)).forEach($ur.removeRule.bind($ur));
// Remove state from registry
delete this.states[state.name];
});

Expand Down
42 changes: 35 additions & 7 deletions src/state/targetState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
* @module state
*/ /** for typedoc */

import {StateDeclaration, StateOrName} from "./interface";
import {ParamsOrArray} from "../params/interface";
import {TransitionOptions} from "../transition/interface";

import {State} from "./stateObject";
import {toJson} from "../common/common";
import { StateDeclaration, StateOrName, TargetStateDef } from "./interface";
import { ParamsOrArray } from "../params/interface";
import { TransitionOptions } from "../transition/interface";
import { State } from "./stateObject";
import { toJson } from "../common/common";
import { isString } from "../common/predicates";

/**
* Encapsulate the target (destination) state/params/options of a [[Transition]].
Expand Down Expand Up @@ -53,48 +53,59 @@ export class TargetState {
* @param _definition The internal state representation, if exists.
* @param _params Parameters for the target state
* @param _options Transition options.
*
* @internalapi
*/
constructor(
private _identifier: StateOrName,
private _definition?: State,
_params: ParamsOrArray = {},
_params?: ParamsOrArray,
private _options: TransitionOptions = {}
) {
this._params = _params || {};
}

/** The name of the state this object targets */
name(): String {
return this._definition && this._definition.name || <String> this._identifier;
}

/** The identifier used when creating this TargetState */
identifier(): StateOrName {
return this._identifier;
}

/** The target parameter values */
params(): ParamsOrArray {
return this._params;
}

/** The internal state object (if it was found) */
$state(): State {
return this._definition;
}

/** The internal state declaration (if it was found) */
state(): StateDeclaration {
return this._definition && this._definition.self;
}

/** The target options */
options() {
return this._options;
}

/** True if the target state was found */
exists(): boolean {
return !!(this._definition && this._definition.self);
}

/** True if the object is valid */
valid(): boolean {
return !this.error();
}

/** If the object is invalid, returns the reason why */
error(): string {
let base = <any> this.options().relative;
if (!this._definition && !!base) {
Expand All @@ -110,4 +121,21 @@ export class TargetState {
toString() {
return `'${this.name()}'${toJson(this.params())}`;
}

/** Returns true if the object has a state property that might be a state or state name */
static isDef = (obj): obj is TargetStateDef =>
obj && obj.state && (isString(obj.state) || isString(obj.state.name));

// /** Returns a new TargetState based on this one, but using the specified options */
// withOptions(_options: TransitionOptions): TargetState {
// return extend(this._clone(), { _options });
// }
//
// /** Returns a new TargetState based on this one, but using the specified params */
// withParams(_params: ParamsOrArray): TargetState {
// return extend(this._clone(), { _params });
// }

// private _clone = () =>
// new TargetState(this._identifier, this._definition, this._params, this._options);
}
Loading

0 comments on commit eb2f5d7

Please sign in to comment.