fix(aio): strip leading slashes from path (and improve DRY-ness) #16238

gkalpak · 2017-04-21T14:31:35Z

Previously, the path returned by LocationService.path() preserved leading slashes, which resulted in requests with consequtive slashes in the URL. Such requests would fail (with a 404) on staging.

This commit fixes it, by removing leading slashes from the path. It also refactors LocationService a bit, converting path to an observable, currentPath (similar to currentUrl), and applies certain clean-ups (e.g. stripping slashes, query, hash) in one place, which simplifies consumption.

This is an alternative to #16230.

petebacondarwin · 2017-04-21T14:48:13Z

This is quite likely to break https://github.com/angular/angular/blob/master/aio/src/app/embedded/current-location.component.ts#L10

petebacondarwin · 2017-04-21T14:48:36Z

But we could refactor that too, to use the async filter etc.

petebacondarwin · 2017-04-21T14:50:00Z

aio/src/app/shared/location.service.spec.ts

  });

-  describe('go', () => {
-    it('should update the location', () => {
+  describe('pathStream', () => {


it is just currentPath - no need for the stream thingy

Was keeping in-line with the urlStream above, but I'll change them both 😃

petebacondarwin · 2017-04-21T14:53:41Z

aio/src/app/shared/location.service.ts

@@ -14,21 +14,22 @@ export class LocationService {
  private readonly urlParser = document.createElement('a');
  private urlSubject = new Subject<string>();
  currentUrl = this.urlSubject
+    .map(url => this.stripSlashes(url))


This will not strip trailing slashes from URLs that have a query or a hash. E.g. /a/b/c/?some=value

petebacondarwin · 2017-04-21T14:55:48Z

aio/src/app/shared/location.service.ts

@@ -122,7 +113,7 @@ export class LocationService {
      return true;
    }

-    this.go(this.stripLeadingSlashes(relativeUrl));
+    this.go(this.stripSlashes(relativeUrl));


perhaps we ought to be doing the slash stripping in the go method, instead?

Sounds reasonable.

petebacondarwin · 2017-04-21T14:56:43Z

aio/src/testing/location.service.ts

@@ -3,12 +3,11 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject';
 export class MockLocationService {
  urlSubject = new BehaviorSubject<string>(this.initialUrl);


do we need to strip slashes here then?

We are not stripping slashes anywhere in this mock implementation. I assume it is the user's responsibility to provide URLs without slashes.

petebacondarwin

It seemed like such a simple refactoring eh?

We need to fix the CurrentLocationComponent usage of this service (and maybe add an e2e test to check); and I think there is a problem with stripping trailing slashes when the url contains a query or a hash.

petebacondarwin · 2017-04-21T15:14:55Z

aio/src/app/shared/location.service.spec.ts

+        const service: LocationService = injector.get(LocationService);
+        let url: string;
+
+        location.simulatePopState('///some/url///');


Now try:

location.simulatePopState('///some/url///?some=query');

petebacondarwin · 2017-04-21T15:15:54Z

aio/src/app/shared/location.service.spec.ts


-      const urls = [];
-      service.currentUrl.subscribe(url => urls.push(url));
+      location.simulatePopState('/initial/url1?foo=bar');


location.simulatePopState('///some/url///?some=query');

petebacondarwin

I like the latest changes 👍
Still need to fix the trailing slashes issue

gkalpak · 2017-04-21T15:47:09Z

@petebd, I think I addressed all comments. PTAL

petebacondarwin · 2017-04-21T15:52:22Z

I'll squash and re-push just to be sure - before merging

gkalpak · 2017-04-21T15:53:28Z

I pushed another commit with more tests after you approved 😁

Previously, the path returned by `LocationService.path()` preserved leading slashes, which resulted in requests with consequtive slashes in the URL. Such requests would fail (with a 404) on staging. This commit fixes it, by removing leading slashes from the path. It also refactors `LocationService` a bit, converting path to an observable, `currentPath` (similar to `currentUrl`), and applies certain clean-ups (e.g. stripping slashes, query, hash) in one place, which simplifies consumption. Closes angular#16230

gkalpak · 2017-04-21T17:11:18Z

Fixed linting issues and squashed.

mary-poppins · 2017-04-21T17:23:46Z

The angular.io preview for 781dce2 is available here.

petebacondarwin · 2017-04-21T17:34:24Z

Great. Let's merge

…ular#16238) Previously, the path returned by `LocationService.path()` preserved leading slashes, which resulted in requests with consequtive slashes in the URL. Such requests would fail (with a 404) on staging. This commit fixes it, by removing leading slashes from the path. It also refactors `LocationService` a bit, converting path to an observable, `currentPath` (similar to `currentUrl`), and applies certain clean-ups (e.g. stripping slashes, query, hash) in one place, which simplifies consumption. Closes angular#16230

angular-automatic-lock-bot · 2019-09-11T09:22:44Z

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: yes label Apr 21, 2017

gkalpak added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 21, 2017

gkalpak added this to REVIEW in docs-infra Apr 21, 2017

petebacondarwin reviewed Apr 21, 2017

View reviewed changes

petebacondarwin requested changes Apr 21, 2017

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 21, 2017

petebacondarwin mentioned this pull request Apr 21, 2017

fix(aio): remove leading slashes from the Location path #16230

Closed

petebacondarwin reviewed Apr 21, 2017

View reviewed changes

petebacondarwin requested changes Apr 21, 2017

View reviewed changes

petebacondarwin approved these changes Apr 21, 2017

View reviewed changes

gkalpak force-pushed the fix-aio-remove-extraneous-slash branch from 8a9f565 to 1f0fbb9 Compare April 21, 2017 17:07

gkalpak force-pushed the fix-aio-remove-extraneous-slash branch from 1f0fbb9 to 781dce2 Compare April 21, 2017 17:09

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 21, 2017

petebacondarwin moved this from REVIEW to MERGE in docs-infra Apr 21, 2017

mhevery merged commit 9c1318d into angular:master Apr 21, 2017

gkalpak deleted the fix-aio-remove-extraneous-slash branch April 21, 2017 20:38

petebacondarwin removed this from MERGE in docs-infra Apr 22, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aio): strip leading slashes from path (and improve DRY-ness) #16238

fix(aio): strip leading slashes from path (and improve DRY-ness) #16238

gkalpak commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

petebacondarwin Apr 21, 2017

gkalpak Apr 21, 2017

petebacondarwin Apr 21, 2017

petebacondarwin Apr 21, 2017

gkalpak Apr 21, 2017

petebacondarwin Apr 21, 2017

gkalpak Apr 21, 2017 •

edited

petebacondarwin left a comment

petebacondarwin Apr 21, 2017

petebacondarwin Apr 21, 2017

petebacondarwin left a comment

gkalpak commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

gkalpak commented Apr 21, 2017

gkalpak commented Apr 21, 2017

mary-poppins commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

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

		@@ -3,12 +3,11 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject';
		export class MockLocationService {
		urlSubject = new BehaviorSubject<string>(this.initialUrl);

fix(aio): strip leading slashes from path (and improve DRY-ness) #16238

fix(aio): strip leading slashes from path (and improve DRY-ness) #16238

Conversation

gkalpak commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Apr 21, 2017 • edited

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

gkalpak commented Apr 21, 2017

gkalpak commented Apr 21, 2017

mary-poppins commented Apr 21, 2017

petebacondarwin commented Apr 21, 2017

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

gkalpak Apr 21, 2017 •

edited