From c589806bc2cb026c4a1e5fd0ba8baa5e10d74073 Mon Sep 17 00:00:00 2001 From: David Reher Date: Fri, 4 Mar 2016 09:14:27 +0100 Subject: [PATCH 1/3] fix(angular_1_router): ng-link is generating wronf hrefs At the moment ng-link is generating html5mode URLs for `href`s. Instead it should check whether or not html5mode is enabled and create the `href`s accordingly. The renaming in the `getLink` function is aligning it to `RouterLink`'s `_updateLink`. --- modules/angular1_router/lib/facades.es5 | 9 +++++++ modules/angular1_router/src/ng_outlet.ts | 11 +++++---- modules/angular1_router/test/ng_link_spec.js | 26 ++++++++++++++++++-- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/modules/angular1_router/lib/facades.es5 b/modules/angular1_router/lib/facades.es5 index 2be12cde285c..c116f12958ba 100644 --- a/modules/angular1_router/lib/facades.es5 +++ b/modules/angular1_router/lib/facades.es5 @@ -316,3 +316,12 @@ Location.prototype.path = function () { Location.prototype.go = function (path, query) { return $location.url(path + query); }; +Location.prototype.prepareExternalUrl = function(url) { + if (url.length > 0 && !url.startsWith('/')) { + url = '/' + url; + } + if(!$location.$$html5) { + url = '#' + url; + } + return url; +}; diff --git a/modules/angular1_router/src/ng_outlet.ts b/modules/angular1_router/src/ng_outlet.ts index f6e346014a93..b7d73e4a2791 100644 --- a/modules/angular1_router/src/ng_outlet.ts +++ b/modules/angular1_router/src/ng_outlet.ts @@ -252,12 +252,13 @@ function ngLinkDirective($rootRouter, $parse) { return; } - let instruction = null; + let navigationInstruction = null; let link = attrs.ngLink || ''; function getLink(params) { - instruction = router.generate(params); - return './' + angular.stringifyInstruction(instruction); + navigationInstruction = router.generate(params); + const navigationHref = navigationInstruction.toLinkUrl(); + return $rootRouter._location.prepareExternalUrl(navigationHref); } let routeParamsGetter = $parse(link); @@ -271,11 +272,11 @@ function ngLinkDirective($rootRouter, $parse) { } element.on('click', event => { - if (event.which !== 1 || !instruction) { + if (event.which !== 1 || !navigationInstruction) { return; } - $rootRouter.navigateByInstruction(instruction); + $rootRouter.navigateByInstruction(navigationInstruction); event.preventDefault(); }); } diff --git a/modules/angular1_router/test/ng_link_spec.js b/modules/angular1_router/test/ng_link_spec.js index 56b613fb6c70..a2d6e00f9548 100644 --- a/modules/angular1_router/test/ng_link_spec.js +++ b/modules/angular1_router/test/ng_link_spec.js @@ -6,15 +6,19 @@ describe('ngLink', function () { $compile, $rootScope, $rootRouter, - $compileProvider; + $compileProvider, + $locationProvider; beforeEach(function () { module('ng'); module('ngComponentRouter'); - module(function (_$compileProvider_) { + module(function (_$compileProvider_, _$locationProvider_) { $compileProvider = _$compileProvider_; + $locationProvider = _$locationProvider_; }); + $locationProvider.html5mode(true); + inject(function (_$compile_, _$rootScope_, _$rootRouter_) { $compile = _$compile_; $rootScope = _$rootScope_; @@ -135,6 +139,24 @@ describe('ngLink', function () { }).not.toThrow(); }); + describe('html5mode disabled', function () { + beforeEach(function () { + $locationProvider.html5mode(false); + }); + + + it('should prepend href with a hash', function () { + $rootRouter.config([ + { path: '/b', component: 'twoCmp', name: 'Two' } + ]); + compile('link'); + + $rootScope.$digest(); + + expect(elt.find('a').attr('href')).toBe('#/b'); + }); + }); + function registerComponent(name, template, config) { var controller = function () {}; From 0382d800dd8a8d9d9de635c2a820c56ad5574887 Mon Sep 17 00:00:00 2001 From: David Reher Date: Tue, 8 Mar 2016 09:32:27 +0100 Subject: [PATCH 2/3] test(angular-1-router): All tests with `href`s are run with and without html5mode --- modules/angular1_router/test/ng_link_spec.js | 141 +++++++++---------- 1 file changed, 67 insertions(+), 74 deletions(-) diff --git a/modules/angular1_router/test/ng_link_spec.js b/modules/angular1_router/test/ng_link_spec.js index 0a02157ae3f2..21faf48af86e 100644 --- a/modules/angular1_router/test/ng_link_spec.js +++ b/modules/angular1_router/test/ng_link_spec.js @@ -29,83 +29,94 @@ describe('ngLink', function () { registerComponent('oneCmp', '
{{oneCmp.number}}
', function () {this.number = 'one'}); registerComponent('twoCmp', '
{{twoCmp.number}}
', function () {this.number = 'two'}); }); + + describe('html5mode enabled', function () { + $locationProvider.html5mode(true); + runHrefTestsAndExpectPrefix('.') + }); + + describe('html5mode disabled', function () { + $locationProvider.html5mode(false); + runHrefTestsAndExpectPrefix('#') + }); + function runHrefTestsAndExpectPrefix(prefix) { + it('should allow linking from the parent to the child', function () { + $rootRouter.config([ + { path: '/a', component: 'oneCmp' }, + { path: '/b', component: 'twoCmp', name: 'Two' } + ]); + compile('link | outer {
}'); - it('should allow linking from the parent to the child', function () { - $rootRouter.config([ - { path: '/a', component: 'oneCmp' }, - { path: '/b', component: 'twoCmp', name: 'Two' } - ]); - compile('link | outer {
}'); - - $rootRouter.navigateByUrl('/a'); - $rootScope.$digest(); + $rootRouter.navigateByUrl('/a'); + $rootScope.$digest(); - expect(elt.find('a').attr('href')).toBe('./b'); - }); + expect(elt.find('a').attr('href')).toBe(prefix + '/b'); + }); - it('should allow linking from the child and the parent', function () { - $rootRouter.config([ - { path: '/a', component: 'oneCmp' }, - { path: '/b', component: 'twoCmp', name: 'Two' } - ]); - compile('outer {
}'); + it('should allow linking from the child and the parent', function () { + $rootRouter.config([ + { path: '/a', component: 'oneCmp' }, + { path: '/b', component: 'twoCmp', name: 'Two' } + ]); + compile('outer {
}'); - $rootRouter.navigateByUrl('/b'); - $rootScope.$digest(); + $rootRouter.navigateByUrl('/b'); + $rootScope.$digest(); - expect(elt.find('a').attr('href')).toBe('./b'); - }); + expect(elt.find('a').attr('href')).toBe(prefix + '/b'); + }); - it('should allow params in routerLink directive', function () { - registerComponent('twoLinkCmp', '
{{twoLinkCmp.number}}
', function () {this.number = 'two'}); + it('should allow params in routerLink directive', function () { + registerComponent('twoLinkCmp', '
{{twoLinkCmp.number}}
', function () {this.number = 'two'}); - $rootRouter.config([ - { path: '/a', component: 'twoLinkCmp' }, - { path: '/b/:param', component: 'twoCmp', name: 'Two' } - ]); - compile('
'); + $rootRouter.config([ + { path: '/a', component: 'twoLinkCmp' }, + { path: '/b/:param', component: 'twoCmp', name: 'Two' } + ]); + compile('
'); - $rootRouter.navigateByUrl('/a'); - $rootScope.$digest(); + $rootRouter.navigateByUrl('/a'); + $rootScope.$digest(); - expect(elt.find('a').attr('href')).toBe('./b/lol'); - }); + expect(elt.find('a').attr('href')).toBe(prefix + '/b/lol'); + }); - it('should update the href of links with bound params', function () { - registerComponent('twoLinkCmp', '
{{twoLinkCmp.number}}
', function () {this.number = 'param'}); - $rootRouter.config([ - { path: '/a', component: 'twoLinkCmp' }, - { path: '/b/:param', component: 'twoCmp', name: 'Two' } - ]); - compile('
'); + it('should update the href of links with bound params', function () { + registerComponent('twoLinkCmp', '
{{twoLinkCmp.number}}
', function () {this.number = 'param'}); + $rootRouter.config([ + { path: '/a', component: 'twoLinkCmp' }, + { path: '/b/:param', component: 'twoCmp', name: 'Two' } + ]); + compile('
'); - $rootRouter.navigateByUrl('/a'); - $rootScope.$digest(); + $rootRouter.navigateByUrl('/a'); + $rootScope.$digest(); - expect(elt.find('a').attr('href')).toBe('./b/param'); - }); + expect(elt.find('a').attr('href')).toBe(pefix + '/b/param'); + }); - it('should navigate on left-mouse click when a link url matches a route', function () { - $rootRouter.config([ - { path: '/', component: 'oneCmp' }, - { path: '/two', component: 'twoCmp', name: 'Two'} - ]); + it('should navigate on left-mouse click when a link url matches a route', function () { + $rootRouter.config([ + { path: '/', component: 'oneCmp' }, + { path: '/two', component: 'twoCmp', name: 'Two'} + ]); - compile('link |
'); - $rootScope.$digest(); - expect(elt.text()).toBe('link | one'); + compile('link |
'); + $rootScope.$digest(); + expect(elt.text()).toBe('link | one'); - expect(elt.find('a').attr('href')).toBe('./two'); + expect(elt.find('a').attr('href')).toBe(prefix + '/two'); - elt.find('a')[0].click(); + elt.find('a')[0].click(); - $rootScope.$digest(); - expect(elt.text()).toBe('link | two'); - }); + $rootScope.$digest(); + expect(elt.text()).toBe('link | two'); + }); + } it('should not navigate on non-left mouse click when a link url matches a route', inject(function ($rootRouter) { @@ -153,24 +164,6 @@ describe('ngLink', function () { expect(elt.find('a').attr('class')).toBe('ng-link-active'); })); - describe('html5mode disabled', function () { - beforeEach(function () { - $locationProvider.html5mode(false); - }); - - - it('should prepend href with a hash', function () { - $rootRouter.config([ - { path: '/b', component: 'twoCmp', name: 'Two' } - ]); - compile('link'); - - $rootScope.$digest(); - - expect(elt.find('a').attr('href')).toBe('#/b'); - }); - }); - function registerComponent(name, template, config) { var controller = function () {}; From 8869e8294b03b57e1d0ce7ad764461f47669922f Mon Sep 17 00:00:00 2001 From: David Reher Date: Tue, 8 Mar 2016 12:15:22 +0100 Subject: [PATCH 3/3] test(angular-1-router): fixed some failing tests --- modules/angular1_router/test/ng_link_spec.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/modules/angular1_router/test/ng_link_spec.js b/modules/angular1_router/test/ng_link_spec.js index 21faf48af86e..23271980ce87 100644 --- a/modules/angular1_router/test/ng_link_spec.js +++ b/modules/angular1_router/test/ng_link_spec.js @@ -14,11 +14,9 @@ describe('ngLink', function () { module('ngComponentRouter'); module(function (_$compileProvider_, _$locationProvider_) { $compileProvider = _$compileProvider_; - $locationProvider = _$locationProvider_; + $locationProvider = _$locationProvider_; }); - $locationProvider.html5mode(true); - inject(function (_$compile_, _$rootScope_, _$rootRouter_) { $compile = _$compile_; $rootScope = _$rootScope_; @@ -31,13 +29,17 @@ describe('ngLink', function () { }); describe('html5mode enabled', function () { - $locationProvider.html5mode(true); - runHrefTestsAndExpectPrefix('.') + beforeEach(function () { + $locationProvider.html5Mode(true); + }); + runHrefTestsAndExpectPrefix('.'); }); describe('html5mode disabled', function () { - $locationProvider.html5mode(false); - runHrefTestsAndExpectPrefix('#') + beforeEach(function () { + $locationProvider.html5Mode(false); + }); + runHrefTestsAndExpectPrefix('#'); }); function runHrefTestsAndExpectPrefix(prefix) { @@ -95,7 +97,7 @@ describe('ngLink', function () { $rootRouter.navigateByUrl('/a'); $rootScope.$digest(); - expect(elt.find('a').attr('href')).toBe(pefix + '/b/param'); + expect(elt.find('a').attr('href')).toBe(prefix + '/b/param'); });