-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Removes hash fragments from url in offline audit #1319
Removes hash fragments from url in offline audit #1319
Conversation
@@ -34,4 +36,8 @@ class Offline extends Gatherer { | |||
} | |||
} | |||
|
|||
function cleanHash(url) { | |||
return url.href.removeURLFragment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar withremoveURLFragment
. Where does it come from?
If it's not part of a spec, url.origin + url.pathname + url.search
would be clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest autocomplete let me to it :p. (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/platform/utilities.js?q=removeURLFragment&sq=package:chromium&l=215)
I can just copy that function instead so we are sure it never disappears. Reason I didn't want to build the url myself is that I keep the href as pure as it can be but maybe it's not necessary at all 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow. Yea, let's go with our own implementation instead of a function added to String's prototoype.
How about url.origin + url.pathname + url.search
? It's obvious and pure 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do the new URL()
part inside here too? Can also do url.hash = ''; return url.href;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay the subclass of URL is now in place, so we can move it there. :)
@@ -25,7 +26,8 @@ class Offline extends Gatherer { | |||
|
|||
afterPass(options, tracingData) { | |||
const navigationRecord = tracingData.networkRecords.filter(record => { | |||
return record._url === options.url && record._fetchedViaServiceWorker; | |||
return cleanHash(new URL(record._url)) === cleanHash(new URL(options.url)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little worrying at first (how do we account for all the crazy things SPAs do with URL fragments?), but this is in line with how service worker caching of the page works (the SW Cache explicitly ignores the URL fragment on requests), so seems like it may be the right thing to do.
@jeffposnick for his opinion and for any cases where this may go awry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the full context, but in general, yeah, I think ignoring the hash
would make sense from the point of view of both the Cache Storage API and navigation requests.
@@ -34,4 +36,8 @@ class Offline extends Gatherer { | |||
} | |||
} | |||
|
|||
function cleanHash(url) { | |||
return url.href.removeURLFragment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do the new URL()
part inside here too? Can also do url.hash = ''; return url.href;
assert.strictEqual(artifact, 200); | ||
}), | ||
offlineGather.afterPass(optionsWithQueryString, tracingData).then(artifact => { | ||
assert.strictEqual(artifact, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a failure test, so maybe move to the other it()
?
To clarify what's happening here:
This is kind of the opposite problem of #715, where a real redirect (not just adding a fragment to the URL) makes the CLI fail the offline test because it tests the original URL but the extension passes since it always tests the redirected URL. |
I think this could sit on top of the subclassed so it could be |
@paulirish that make sense :) @brendankenny maybe update the audit to check url with fragment and without? |
#1390 landed, so good to go on adding method to the url shim |
11229bc
to
5aeabec
Compare
I've 2 options. I went for option 1 Option 1: /**
* Removes hash from URL
* @return {URL} Url object chaining
*/
URL.prototype.stripHash = function stripHash() {
this.hash = '';
return this;
}; Option 2: /**
* Removes hash from an url and returns it
* @return {string} Url without the hash
*/
URL.prototype.stripHash = function stripHash() {
const currentHash = this.hash;
this.hash = '';
const urlWithoutHash = this.href;
this.hash = currentHash;
return urlWithoutHash;
}; |
a third option would mirror the URL spec's equality with the exclude fragments flag set. We could name it something awkward like /**
* Determine if url1 equals url2, ignoring URL fragments.
* @param {string} url1
* @param {string} url2
* @return {boolean}
*/
URL.equalWithExcludedFragments = function(url1, url2) {
url1 = new URL(url1);
url1.hash = '';
url2 = new URL(url2);
url2.hash = '';
return url1.href === url2.href;
}; (or pick a much better name :) |
@brendankenny do you rather have option 3? |
I would, but don't know what others think |
* Removes hash from URL | ||
* @return {URL} Url object chaining | ||
*/ | ||
URL.prototype.stripHash = function stripHash() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need test(s) in url-shim-test
for whatever we end up putting in here
+1 to a static method over a prototype extension, but I'm not terribly invested in it and fine with this too. |
equalWithExcludedFragments it will be :) |
yay awkward naming :) |
@wardpeet :):) what do you think? |
i'll try to update today :) |
@wardpeet can you rebase? |
…ne check. We store the url inside options.url without a fragment so we should do this check without fragment as well. Fixes PR GoogleChrome#1296
Updated finally, sorry for the wait! |
b466513
to
4e85708
Compare
Can you add a test for equalWithExcludedFragments in url-shim-test.js? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wardpeet I'm going to add the tests for you :P
@brendankenny thanks |
Reviewers: ALL
We store the url inside options.url without a fragment so we should do this check without fragment as well.
Make sure that I don't break anything crucial :p
Fixes PR #1296