Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(router-plugin): serialize entered URL and compare with the recognized one #1159

Merged
merged 9 commits into from
Jul 19, 2019

Conversation

arturovt
Copy link
Member

PR Checklist

PR Type

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1153

What is the new behavior?

location.pathname is concatenated with location.search as it has to be.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@arturovt arturovt requested review from markwhitfeld and splincode and removed request for markwhitfeld July 17, 2019 18:21
@arturovt
Copy link
Member Author

@splincode @markwhitfeld

Just a small bugfix.

Copy link
Member

@splincode splincode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test?)

@arturovt
Copy link
Member Author

Test?)

if (isAngularInTestMode()) {
  return;
}

?

@splincode
Copy link
Member

I mean, can you write tests?

@markwhitfeld
Copy link
Member

Thanks @arturovt!
Just a thought, could this be done using this method instead?
https://angular.io/api/common/Location#iscurrentpathequalto
This could make it more resilient around urls that are the same but differ in small aspects like some url encoded characters. Really unfamiliar with this API though, so this is a guess.

@markwhitfeld
Copy link
Member

Oh, and can you remember what this code was a workaround for?

if (isAngularInTestMode()) {
  return;
}

If it was around protecting against no window.location in nodejs (jest) then we have fixed that shortfall by using PlatformLocation.

@arturovt
Copy link
Member Author

Thanks @arturovt!
Just a thought, could this be done using this method instead?
https://angular.io/api/common/Location#iscurrentpathequalto
This could make it more resilient around urls that are the same but differ in small aspects like some url encoded characters. Really unfamiliar with this API though, so this is a guess.

Let me check tomorrow.

@arturovt
Copy link
Member Author

arturovt commented Jul 18, 2019

@markwhitfeld

The problem of isCurrentPathEqualTo is that we basically compare the entered URL with itself, like:

// entered in the search bar
const url = '/path';
if (url === url) {
  ... do stuff
}

So this code:

 const currentUrl = `${this._platformLocation.pathname}${this._platformLocation.search}`;
if (this._location.isCurrentPathEqualTo(currentUrl)) {
  // ...
}

Is not valid, because Angular compares pathname + search with pathname + search, because isCurrentPathEqualTo uses PlatformLocation under the hood.

By using the url from the RoutesRecognized event - we're comparing the recognized one URL.

If we'd use isCurrentPathEqualTo - that means that if the user enters /path in the address bar - but router plugin recognized another URL - the user will be redirected to the previously saved URL.

We could use isCurrentPathEqualTo with the recognized URL, but the behavior will be the same...
I can re-write this code, this is not a problem.

@markwhitfeld
Copy link
Member

@arturovt Are you saying that we could write it as:

if (this._location.isCurrentPathEqualTo(url)) {
  // ...
}

@arturovt arturovt changed the title fix(router-plugin): concat pathname with search fix(router-plugin): serialize entered URL and compare with the recognized one Jul 19, 2019
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nicely done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants