From b30bb8dd91c61684f16664e6ae6589113737a76d Mon Sep 17 00:00:00 2001 From: Zaid Al-Omari Date: Fri, 9 Mar 2018 03:29:55 +0400 Subject: [PATCH] fix(router): make routerLinkActive work with query params which contain arrays (#22666) The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly fixes #22223 PR Close #22666 --- packages/router/src/shared.ts | 2 +- packages/router/src/url_tree.ts | 4 ++-- packages/router/src/utils/collection.ts | 18 ++++++++++++--- packages/router/test/url_tree.spec.ts | 30 +++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/packages/router/src/shared.ts b/packages/router/src/shared.ts index 8a0c4c3b96..51c861cc8f 100644 --- a/packages/router/src/shared.ts +++ b/packages/router/src/shared.ts @@ -25,7 +25,7 @@ export const PRIMARY_OUTLET = 'primary'; * @publicApi */ export type Params = { - [key: string]: any + [key: string]: any; }; /** diff --git a/packages/router/src/url_tree.ts b/packages/router/src/url_tree.ts index 3ea9dcf231..94fc9f0b37 100644 --- a/packages/router/src/url_tree.ts +++ b/packages/router/src/url_tree.ts @@ -7,7 +7,7 @@ */ import {PRIMARY_OUTLET, ParamMap, Params, convertToParamMap} from './shared'; -import {forEach, shallowEqual} from './utils/collection'; +import {equalArraysOrString, forEach, shallowEqual} from './utils/collection'; export function createEmptyUrlTree() { return new UrlTree(new UrlSegmentGroup([], {}), {}, null); @@ -41,7 +41,7 @@ function equalSegmentGroups(container: UrlSegmentGroup, containee: UrlSegmentGro function containsQueryParams(container: Params, containee: Params): boolean { // TODO: This does not handle array params correctly. return Object.keys(containee).length <= Object.keys(container).length && - Object.keys(containee).every(key => containee[key] === container[key]); + Object.keys(containee).every(key => equalArraysOrString(container[key], containee[key])); } function containsSegmentGroup(container: UrlSegmentGroup, containee: UrlSegmentGroup): boolean { diff --git a/packages/router/src/utils/collection.ts b/packages/router/src/utils/collection.ts index d6c47dda19..74fbc81420 100644 --- a/packages/router/src/utils/collection.ts +++ b/packages/router/src/utils/collection.ts @@ -10,7 +10,7 @@ import {NgModuleFactory, ɵisObservable as isObservable, ɵisPromise as isPromis import {Observable, from, of } from 'rxjs'; import {concatAll, last as lastValue, map} from 'rxjs/operators'; -import {PRIMARY_OUTLET} from '../shared'; +import {PRIMARY_OUTLET, Params} from '../shared'; export function shallowEqualArrays(a: any[], b: any[]): boolean { if (a.length !== b.length) return false; @@ -20,7 +20,7 @@ export function shallowEqualArrays(a: any[], b: any[]): boolean { return true; } -export function shallowEqual(a: {[x: string]: any}, b: {[x: string]: any}): boolean { +export function shallowEqual(a: Params, b: Params): boolean { // Casting Object.keys return values to include `undefined` as there are some cases // in IE 11 where this can happen. Cannot provide a test because the behavior only // exists in certain circumstances in IE 11, therefore doing this cast ensures the @@ -33,13 +33,25 @@ export function shallowEqual(a: {[x: string]: any}, b: {[x: string]: any}): bool let key: string; for (let i = 0; i < k1.length; i++) { key = k1[i]; - if (a[key] !== b[key]) { + if (!equalArraysOrString(a[key], b[key])) { return false; } } return true; } +/** + * Test equality for arrays of strings or a string. + */ +export function equalArraysOrString(a: string | string[], b: string | string[]) { + if (Array.isArray(a) && Array.isArray(b)) { + if (a.length != b.length) return false; + return a.every(aItem => b.indexOf(aItem) > -1); + } else { + return a === b; + } +} + /** * Flattens single-level nested arrays. */ diff --git a/packages/router/test/url_tree.spec.ts b/packages/router/test/url_tree.spec.ts index 2b46cb1add..d6a31145b4 100644 --- a/packages/router/test/url_tree.spec.ts +++ b/packages/router/test/url_tree.spec.ts @@ -41,6 +41,24 @@ describe('UrlTree', () => { expect(containsTree(t1, t2, true)).toBe(true); }); + it('should return true when queryParams are the same but with diffrent order', () => { + const t1 = serializer.parse('/one/two?test=1&page=5'); + const t2 = serializer.parse('/one/two?page=5&test=1'); + expect(containsTree(t1, t2, true)).toBe(true); + }); + + it('should return true when queryParams contains array params that are the same', () => { + const t1 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=6'); + const t2 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=6'); + expect(containsTree(t1, t2, true)).toBe(true); + }); + + it('should return false when queryParams contains array params but are not the same', () => { + const t1 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=6'); + const t2 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=7'); + expect(containsTree(t1, t2, false)).toBe(false); + }); + it('should return false when queryParams are not the same', () => { const t1 = serializer.parse('/one/two?test=1&page=5'); const t2 = serializer.parse('/one/two?test=1'); @@ -133,6 +151,18 @@ describe('UrlTree', () => { expect(containsTree(t1, t2, false)).toBe(false); }); + it('should return true when container has array params but containee does not have', () => { + const t1 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=6'); + const t2 = serializer.parse('/one/two?test=a&test=b'); + expect(containsTree(t1, t2, false)).toBe(true); + }); + + it('should return false when containee has array params but container does not have', () => { + const t1 = serializer.parse('/one/two?test=a&test=b'); + const t2 = serializer.parse('/one/two?test=a&test=b&pages=5&pages=6'); + expect(containsTree(t1, t2, false)).toBe(false); + }); + it('should return false when containee has different queryParams', () => { const t1 = serializer.parse('/one/two?page=5'); const t2 = serializer.parse('/one/two?test=1');