From ba1e25f53ff867f2caaa3eb3377abfdbea6fedeb Mon Sep 17 00:00:00 2001 From: Anthony Humes Date: Tue, 7 Nov 2017 10:21:48 -0700 Subject: [PATCH] fix(router): take base uri into account in `setUpLocationSync()` (#20244) Normalize the full URL (including the base uri) before passing it to `router.navigateByUrl()`. Fixes #20061 PR Close #20244 --- karma-js.conf.js | 2 +- packages/router/BUILD.bazel | 2 +- packages/router/karma-test-shim.js | 2 + packages/router/karma.conf.js | 9 +- packages/router/upgrade/BUILD.bazel | 9 +- packages/router/upgrade/src/upgrade.ts | 45 ++++++++- packages/router/upgrade/test/BUILD.bazel | 21 ++++ packages/router/upgrade/test/upgrade.spec.ts | 101 +++++++++++++++++++ 8 files changed, 180 insertions(+), 11 deletions(-) create mode 100644 packages/router/upgrade/test/BUILD.bazel create mode 100644 packages/router/upgrade/test/upgrade.spec.ts diff --git a/karma-js.conf.js b/karma-js.conf.js index 255efd856b..f9ecc2b27f 100644 --- a/karma-js.conf.js +++ b/karma-js.conf.js @@ -84,7 +84,7 @@ module.exports = function(config) { 'dist/all/@angular/elements/schematics/**', 'dist/all/@angular/examples/**/e2e_test/*', 'dist/all/@angular/language-service/**', - 'dist/all/@angular/router/test/**', + 'dist/all/@angular/router/**/test/**', 'dist/all/@angular/platform-browser/testing/e2e_util.js', 'dist/all/angular1_router.js', 'dist/examples/**/e2e_test/**', diff --git a/packages/router/BUILD.bazel b/packages/router/BUILD.bazel index 9a73da61c4..17ecee93c9 100644 --- a/packages/router/BUILD.bazel +++ b/packages/router/BUILD.bazel @@ -15,8 +15,8 @@ ng_module( "//packages/common", "//packages/core", "//packages/platform-browser", - "//packages/upgrade/static", "@rxjs", + "@rxjs//operators", ], ) diff --git a/packages/router/karma-test-shim.js b/packages/router/karma-test-shim.js index 060ab92f1c..2d4304e5a0 100644 --- a/packages/router/karma-test-shim.js +++ b/packages/router/karma-test-shim.js @@ -51,6 +51,8 @@ System.config({ '@angular/platform-browser': {main: 'index.js', defaultExtension: 'js'}, '@angular/platform-browser-dynamic/testing': {main: 'index.js', defaultExtension: 'js'}, '@angular/platform-browser-dynamic': {main: 'index.js', defaultExtension: 'js'}, + '@angular/upgrade/static': {main: 'index.js', defaultExtension: 'js'}, + '@angular/router/upgrade': {main: 'index.js', defaultExtension: 'js'}, '@angular/router/testing': {main: 'index.js', defaultExtension: 'js'}, '@angular/router': {main: 'index.js', defaultExtension: 'js'}, 'rxjs/ajax': {main: 'index.js', defaultExtension: 'js'}, diff --git a/packages/router/karma.conf.js b/packages/router/karma.conf.js index 05b0a737f6..758973b9ce 100644 --- a/packages/router/karma.conf.js +++ b/packages/router/karma.conf.js @@ -61,21 +61,24 @@ module.exports = function(config) { { pattern: 'dist/all/@angular/platform-browser/testing/**/*.js', included: false, - watched: false, + watched: false }, {pattern: 'dist/all/@angular/platform-browser-dynamic/*.js', included: false, watched: false}, { pattern: 'dist/all/@angular/platform-browser-dynamic/src/**/*.js', included: false, - watched: false, + watched: false }, { pattern: 'dist/all/@angular/platform-browser-dynamic/testing/**/*.js', included: false, - watched: false, + watched: false }, + {pattern: 'dist/all/@angular/upgrade/static/*.js', included: false, watched: false}, + {pattern: 'dist/all/@angular/upgrade/static/src/**/*.js', included: false, watched: false}, + // Router {pattern: 'dist/all/@angular/router/**/*.js', included: false, watched: true} ], diff --git a/packages/router/upgrade/BUILD.bazel b/packages/router/upgrade/BUILD.bazel index 6c3889062b..8620441e26 100644 --- a/packages/router/upgrade/BUILD.bazel +++ b/packages/router/upgrade/BUILD.bazel @@ -6,10 +6,17 @@ load("//tools:defaults.bzl", "ng_module") ng_module( name = "upgrade", - srcs = glob(["**/*.ts"]), + srcs = glob( + [ + "*.ts", + "src/**/*.ts", + ], + ), module_name = "@angular/router/upgrade", deps = [ + "//packages/common", "//packages/core", "//packages/router", + "//packages/upgrade/static", ], ) diff --git a/packages/router/upgrade/src/upgrade.ts b/packages/router/upgrade/src/upgrade.ts index 184cf72b58..bd44c6310c 100644 --- a/packages/router/upgrade/src/upgrade.ts +++ b/packages/router/upgrade/src/upgrade.ts @@ -6,12 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ +import {Location} from '@angular/common'; import {APP_BOOTSTRAP_LISTENER, ComponentRef, InjectionToken} from '@angular/core'; import {Router} from '@angular/router'; import {UpgradeModule} from '@angular/upgrade/static'; - - /** * @description * @@ -68,11 +67,47 @@ export function setUpLocationSync(ngUpgrade: UpgradeModule) { } const router: Router = ngUpgrade.injector.get(Router); - const url = document.createElement('a'); + const location: Location = ngUpgrade.injector.get(Location); ngUpgrade.$injector.get('$rootScope') .$on('$locationChangeStart', (_: any, next: string, __: string) => { - url.href = next; - router.navigateByUrl(url.pathname + url.search + url.hash); + const url = resolveUrl(next); + const path = location.normalize(url.pathname); + router.navigateByUrl(path + url.search + url.hash); }); } + +/** + * Normalize and parse a URL. + * + * - Normalizing means that a relative URL will be resolved into an absolute URL in the context of + * the application document. + * - Parsing means that the anchor's `protocol`, `hostname`, `port`, `pathname` and related + * properties are all populated to reflect the normalized URL. + * + * While this approach has wide compatibility, it doesn't work as expected on IE. On IE, normalizing + * happens similar to other browsers, but the parsed components will not be set. (E.g. if you assign + * `a.href = 'foo'`, then `a.protocol`, `a.host`, etc. will not be correctly updated.) + * We work around that by performing the parsing in a 2nd step by taking a previously normalized URL + * and assigning it again. This correctly populates all properties. + * + * See + * https://github.com/angular/angular.js/blob/2c7400e7d07b0f6cec1817dab40b9250ce8ebce6/src/ng/urlUtils.js#L26-L33 + * for more info. + */ +let anchor: HTMLAnchorElement|undefined; +function resolveUrl(url: string): {pathname: string, search: string, hash: string} { + if (!anchor) { + anchor = document.createElement('a'); + } + + anchor.setAttribute('href', url); + anchor.setAttribute('href', anchor.href); + + return { + // IE does not start `pathname` with `/` like other browsers. + pathname: `/${anchor.pathname.replace(/^\//, '')}`, + search: anchor.search, + hash: anchor.hash + }; +} diff --git a/packages/router/upgrade/test/BUILD.bazel b/packages/router/upgrade/test/BUILD.bazel new file mode 100644 index 0000000000..9486783a34 --- /dev/null +++ b/packages/router/upgrade/test/BUILD.bazel @@ -0,0 +1,21 @@ +load("//tools:defaults.bzl", "ts_library", "ts_web_test_suite") + +ts_library( + name = "test_lib", + testonly = 1, + srcs = glob(["**/*.ts"]), + deps = [ + "//packages/common", + "//packages/core/testing", + "//packages/router", + "//packages/router/upgrade", + "//packages/upgrade/static", + ], +) + +ts_web_test_suite( + name = "test_web", + deps = [ + ":test_lib", + ], +) diff --git a/packages/router/upgrade/test/upgrade.spec.ts b/packages/router/upgrade/test/upgrade.spec.ts new file mode 100644 index 0000000000..abbb7cb9a8 --- /dev/null +++ b/packages/router/upgrade/test/upgrade.spec.ts @@ -0,0 +1,101 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Location} from '@angular/common'; +import {TestBed} from '@angular/core/testing'; +import {Router} from '@angular/router'; +import {setUpLocationSync} from '@angular/router/upgrade'; +import {UpgradeModule} from '@angular/upgrade/static'; + +describe('setUpLocationSync', () => { + let upgradeModule: UpgradeModule; + let RouterMock: any; + let LocationMock: any; + + beforeEach(() => { + RouterMock = jasmine.createSpyObj('Router', ['navigateByUrl']); + LocationMock = jasmine.createSpyObj('Location', ['normalize']); + + TestBed.configureTestingModule({ + providers: [ + UpgradeModule, {provide: Router, useValue: RouterMock}, + {provide: Location, useValue: LocationMock} + ], + }); + + upgradeModule = TestBed.get(UpgradeModule); + upgradeModule.$injector = { + get: jasmine.createSpy('$injector.get').and.returnValue({'$on': () => undefined}) + }; + }); + + it('should throw an error if the UpgradeModule.bootstrap has not been called', () => { + upgradeModule.$injector = null; + + expect(() => setUpLocationSync(upgradeModule)).toThrowError(` + RouterUpgradeInitializer can be used only after UpgradeModule.bootstrap has been called. + Remove RouterUpgradeInitializer and call setUpLocationSync after UpgradeModule.bootstrap. + `); + }); + + it('should get the $rootScope from AngularJS and set an $on watch on $locationChangeStart', + () => { + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + + upgradeModule.$injector.get.and.callFake( + (name: string) => (name === '$rootScope') && $rootScope); + + setUpLocationSync(upgradeModule); + + expect($rootScope.$on).toHaveBeenCalledTimes(1); + expect($rootScope.$on).toHaveBeenCalledWith('$locationChangeStart', jasmine.any(Function)); + }); + + it('should navigate by url every time $locationChangeStart is broadcasted', () => { + const url = 'https://google.com'; + const pathname = '/custom/route'; + const normalizedPathname = 'foo'; + const query = '?query=1&query2=3'; + const hash = '#new/hash'; + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + + upgradeModule.$injector.get.and.returnValue($rootScope); + LocationMock.normalize.and.returnValue(normalizedPathname); + + setUpLocationSync(upgradeModule); + + const callback = $rootScope.$on.calls.argsFor(0)[1]; + callback({}, url + pathname + query + hash, ''); + + expect(LocationMock.normalize).toHaveBeenCalledTimes(1); + expect(LocationMock.normalize).toHaveBeenCalledWith(pathname); + + expect(RouterMock.navigateByUrl).toHaveBeenCalledTimes(1); + expect(RouterMock.navigateByUrl).toHaveBeenCalledWith(normalizedPathname + query + hash); + }); + + it('should work correctly on browsers that do not start pathname with `/`', () => { + const anchorProto = HTMLAnchorElement.prototype; + const originalDescriptor = Object.getOwnPropertyDescriptor(anchorProto, 'pathname'); + Object.defineProperty(anchorProto, 'pathname', {get: () => 'foo/bar'}); + + try { + const $rootScope = jasmine.createSpyObj('$rootScope', ['$on']); + upgradeModule.$injector.get.and.returnValue($rootScope); + + setUpLocationSync(upgradeModule); + + const callback = $rootScope.$on.calls.argsFor(0)[1]; + callback({}, '', ''); + + expect(LocationMock.normalize).toHaveBeenCalledWith('/foo/bar'); + } finally { + Object.defineProperty(anchorProto, 'pathname', originalDescriptor !); + } + }); +});