Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

refactor($location): move repeated path normalization code into helper method #16618

Merged
merged 1 commit into from Jul 21, 2018

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Jun 28, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor to reduce duplicate code

Other information:
I originally did this as part of an attempted solution in #16611, but I thought it might be worth committing on it's own.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

One minor suggestion. Otherwise LGTM.

this.$$absUrl = appBaseNoFile + this.$$url.substr(1); // first char is always '/'

this.$$urlUpdatedByLocation = true;
this.$$normalizeUrl = function(path) {
Copy link
Member

Choose a reason for hiding this comment

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

I think $$normalizeUrl's argument should be named url (not path), because that is what it is (in $location terms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... even though it's actually (part of) a path? The main thing this method does is append a base path to the argument. Seems like this is more of a path2url method...

Copy link
Member

Choose a reason for hiding this comment

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

It is not necessarilt part of the path. It also contains query and hash fragment. So, technically it is part the browser URL.
But in $location terms, it is the URL (which is why $$normalizeUrl() is called with $location.$$url 😁).

@jbedard
Copy link
Contributor Author

jbedard commented Jul 19, 2018

Updated

@jbedard jbedard force-pushed the 16100-refactor branch 2 times, most recently from 2329f97 to db77c39 Compare July 20, 2018 03:44
@jbedard jbedard merged commit ff60b3f into angular:master Jul 21, 2018
gkalpak pushed a commit that referenced this pull request Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants