#5502 Bug: Don't prepend unnecessary '/' to path by AlexBachmann · Pull Request #6729 · angular/angular

AlexBachmann · 2016-01-27T20:20:43Z

This is a fix to the Child Router Issue as described in #5502.

I reproduced this bug in this Plunkr
http://plnkr.co/edit/OjXZK9

You will not get errors thrown displayed in the console while running in Plunkr so here is the exact same code on our servers
http://test.loop-one.de/

googlebot · 2016-01-27T20:20:45Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

AlexBachmann · 2016-01-27T20:25:28Z

I signed it.

googlebot · 2016-01-27T20:25:31Z

CLAs look good, thanks!

petebacondarwin · 2016-01-29T12:37:30Z

@Batch1211 - thanks for this but it will need a unit test demonstrating the bug before it can be landed

ericmartinezr · 2016-01-29T12:43:07Z

There's another PR for this #5724 not sure if the case mentioned in both issues are the same

AlexBachmann · 2016-01-30T08:03:27Z

@petebacondarwin I would love to, but it is really hard to get the angular test suit (karma) working on my local machine. Is there documentation provided anywhere on how to set this all up?

AlexBachmann · 2016-01-30T08:09:48Z

@petebacondarwin found it.

petebacondarwin · 2016-01-30T15:22:02Z

@Batch1211 somewhere around here is probably where the test needs to go:

angular/modules/angular1_router/test/integration/navigation_spec.js

Lines 136 to 206 in 6ddfff5

    
           it('should change location path', inject(function ($location) { 
        
             $router.config([ 
        
               { path: '/user', component: 'userCmp' } 
        
             ]); 
        
             compile('<div ng-outlet></div>'); 
        
             $router.navigateByUrl('/user'); 
        
             $rootScope.$digest(); 
        
             expect($location.path()).toBe('/user'); 
        
           })); 
        
           it('should change location to the canonical route', inject(function ($location) { 
        
             compile('<div ng-outlet></div>'); 
        
             $router.config([ 
        
               { path: '/',     redirectTo: ['/User'] }, 
        
               { path: '/user', component:  'userCmp', name: 'User' } 
        
             ]); 
        
             $router.navigateByUrl('/'); 
        
             $rootScope.$digest(); 
        
             expect($location.path()).toBe('/user'); 
        
           })); 
        
           it('should change location to the canonical route with nested components', inject(function ($location) { 
        
             registerComponent('childRouter', { 
        
               template: '<div>inner { <div ng-outlet></div> }</div>', 
        
               $routeConfig: [ 
        
                 { path: '/new-child', component: 'oneCmp', name: 'NewChild'}, 
        
                 { path: '/new-child-two', component: 'twoCmp', name: 'NewChildTwo'} 
        
               ] 
        
             }); 
        
             $router.config([ 
        
               { path: '/old-parent/old-child', redirectTo: ['/NewParent', 'NewChild'] }, 
        
               { path: '/old-parent/old-child-two', redirectTo: ['/NewParent', 'NewChildTwo'] }, 
        
               { path: '/new-parent/...', component:  'childRouter', name: 'NewParent' } 
        
             ]); 
        
             compile('<div ng-outlet></div>'); 
        
             $router.navigateByUrl('/old-parent/old-child'); 
        
             $rootScope.$digest(); 
        
             expect($location.path()).toBe('/new-parent/new-child'); 
        
             expect(elt.text()).toBe('inner { one }'); 
        
             $router.navigateByUrl('/old-parent/old-child-two'); 
        
             $rootScope.$digest(); 
        
             expect($location.path()).toBe('/new-parent/new-child-two'); 
        
             expect(elt.text()).toBe('inner { two }'); 
        
           })); 
        
           it('should navigate when the location path changes', inject(function ($location) { 
        
             $router.config([ 
        
               { path: '/one', component: 'oneCmp' } 
        
             ]); 
        
             compile('<div ng-outlet></div>'); 
        
             $location.path('/one'); 
        
             $rootScope.$digest(); 
        
             expect(elt.text()).toBe('one'); 
        
           }));

You can run these tests from the root of the angular2 project by doing:

gulp test.unit.router

AlexBachmann · 2016-01-30T18:53:20Z

@petebacondarwin Here is a failing unit test for you. This pull request will fix the issue.

One question for my understanding: I realize that the angular1_router uses the same codebase as the angular2 router. But why was I supposed to write the unit test for the angular1_router test suite?

petebacondarwin · 2016-01-30T19:17:55Z

Oh good point. I might have pointed you to the wrong place :-)
On 30 Jan 2016 18:54, "Alexander Bachmann" notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin Here is a failing
unit test for you. This pull request will fix the issue.

One question for my understanding: I realize that the angular1_router uses
the same codebase as the angular2 router. But why was I supposed to write
the unit test for the angular1_router test suite?

—
Reply to this email directly or view it on GitHub
#6729 (comment).

AlexBachmann · 2016-01-30T20:33:35Z

@petebacondarwin I added the second unit test

btford · 2016-02-01T20:24:46Z

LGTM, @Batch1211 can you squash these into a single commit?

AlexBachmann · 2016-02-01T20:34:53Z

@btford Yes I can. But just for my understanding: Why is that helpful?

btford · 2016-02-01T20:54:54Z

It's easier to understand the history (git log) with this fix and tests as a single, atomic change.

In short, it helps communicate the intent of the change better, and makes it easier for others to see what the "finished" change is.

petebacondarwin · 2016-02-01T21:17:45Z

@btford can we not leave the PR as is and squash it when we merge?

alexeagle · 2016-02-01T23:04:19Z

Please clean up the history by squashing the commits:
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request

googlebot · 2016-02-02T05:19:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

AlexBachmann · 2016-02-02T05:23:03Z

Oh no. I made it worse. I'm sorry. I'm not a git-expert when it comes to squashing and rebasing. I'll create a new pull request, where everything is in one commit.

Added unit test for angular2 router test suite This is the the second unit test for the #6729 pull request, which in turn is a fix for the #5502 issue This change fixes the pushState Problem Added unit test for angular2 router test suite This is the the second unit test for the #6729 pull request, which in turn is a fix for the #5502 issue Squashing some commits

googlebot · 2016-02-02T05:42:19Z

CLAs look good, thanks!

AlexBachmann · 2016-02-02T05:44:18Z

@btford Please tell me, that this is correct now :)

petebacondarwin · 2016-02-05T22:11:10Z

modules/angular2/src/router/router.ts

    var emitPath = instruction.toUrlPath();
    var emitQuery = instruction.toUrlQuery();
-    if (emitPath.length > 0) {
+    if (emitPath.length > 0 && emitPath[0] !== '/') {


I wonder if we are allowed to do this without using some StringWrapper method?

I checked with ts2dart and the online dartlang testbed and this is fine.

Apparently we need to use != to compare the strings as in Dart equivalent strings are not always identical

Could we file an issue in ts2dart to detect the unsupported code pattern so we don't fall victim to it again?

@tbosch already did! dart-archive/ts2dart#262

Closes angular#6729 Closes angular#5502

angular-automatic-lock-bot · 2019-09-08T00:20:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: no label Jan 27, 2016

googlebot added cla: yes and removed cla: no labels Jan 27, 2016

petebacondarwin added comp: router and removed comp: router labels Jan 29, 2016

petebacondarwin added the type: bug/fix label Jan 29, 2016

petebacondarwin mentioned this pull request Jan 29, 2016

fix(RootRouter): Path prefixed with a slash when not needed #5724

Closed

AlexBachmann mentioned this pull request Jan 30, 2016

#6729 add unit test #6788

Closed

vsavkin assigned btford Feb 1, 2016

vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 1, 2016

googlebot added cla: no and removed cla: yes labels Feb 2, 2016

googlebot added cla: yes and removed cla: no labels Feb 2, 2016

brandonroberts mentioned this pull request Feb 5, 2016

Router: Failed to execute 'pushState' on 'History' error with PathLocationStrategy #6909

Closed

petebacondarwin reviewed Feb 5, 2016
View reviewed changes

petebacondarwin changed the title ~~#5502 Bug: Child Routers on Root Components with path '/' must throw pushstate error~~ #5502 Bug: Don't prepend unnecessary '/' to path Feb 7, 2016

petebacondarwin added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 7, 2016

petebacondarwin pushed a commit to petebacondarwin/angular that referenced this pull request Feb 8, 2016

fix(router): don't prepend / unnecessarily to Location paths

814cf0b

Closes angular#6729 Closes angular#5502

IgorMinar closed this in c603643 Feb 9, 2016

ericmartinezr mentioned this pull request Mar 16, 2016

angular_1_router: double slash at the root when path is the root itself #7633

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Conversation

AlexBachmann commented Jan 27, 2016

Uh oh!

googlebot commented Jan 27, 2016

Uh oh!

AlexBachmann commented Jan 27, 2016

Uh oh!

googlebot commented Jan 27, 2016

Uh oh!

petebacondarwin commented Jan 29, 2016

Uh oh!

ericmartinezr commented Jan 29, 2016

Uh oh!

AlexBachmann commented Jan 30, 2016

Uh oh!

AlexBachmann commented Jan 30, 2016

Uh oh!

petebacondarwin commented Jan 30, 2016

Uh oh!

AlexBachmann commented Jan 30, 2016

Uh oh!

petebacondarwin commented Jan 30, 2016

Uh oh!

AlexBachmann commented Jan 30, 2016

Uh oh!

btford commented Feb 1, 2016

Uh oh!

AlexBachmann commented Feb 1, 2016

Uh oh!

btford commented Feb 1, 2016

Uh oh!

petebacondarwin commented Feb 1, 2016

Uh oh!

alexeagle commented Feb 1, 2016

Uh oh!

googlebot commented Feb 2, 2016

Uh oh!

AlexBachmann commented Feb 2, 2016

Uh oh!

googlebot commented Feb 2, 2016

Uh oh!

AlexBachmann commented Feb 2, 2016

Uh oh!

petebacondarwin Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 8, 2016

Choose a reason for hiding this comment

Uh oh!

alexeagle Feb 8, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 8, 2016

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 8, 2016

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants