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

URL: ensure deleting all params keeps ? in URL #6445

Merged
merged 1 commit into from
Jul 7, 2017
Merged

URL: ensure deleting all params keeps ? in URL #6445

merged 1 commit into from
Jul 7, 2017

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jul 1, 2017

@ghost
Copy link

ghost commented Jul 1, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision da797a5
Using browser at version BuildID 20170612100241; SourceStamp 27cad9749cddf68e11fdd4e5d73dad84a8f8cf23
Starting 10 test iterations
All results were stable

All results

1 test ran
/url/urlsearchparams-delete.html
Subtest Results Messages
OK
Delete basics PASS
Deleting appended multiple PASS
Deleting all params keeps ? in URL FAIL assert_equals: url.href has ? expected "http://example.com/?" but got "http://example.com/"

@ghost
Copy link

ghost commented Jul 1, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision da797a5
Using browser at version 10.0
Starting 10 test iterations
No tests run.

@ghost
Copy link

ghost commented Jul 1, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision da797a5
Using browser at version 61.0.3141.7 dev
Starting 10 test iterations
No tests run.

@ghost
Copy link

ghost commented Jul 1, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision da797a5
Using browser at version 14.14393
Starting 10 test iterations
No tests run.

@GPHemsley
Copy link
Contributor

GPHemsley commented Jul 1, 2017

Despite what the spec might suggest, neither Firefox nor Chrome keep the ? around in href:

image

image

@annevk
Copy link
Member

annevk commented Jul 1, 2017

It might be worth changing this given what browsers do. It does seem a little cleaner.

@TimothyGu
Copy link
Member Author

@annevk I'll check what Edge does. It is cleaner, but less consistent with the fact that generally the update step is equivalent to url.search = "?" + url.searchParams.toString().

@zimbabao
Copy link

zimbabao commented Jul 3, 2017

Deleting non existing param gives different behavior on browsers

  1. Chrome: Removes '?'
  2. Safari/Firefox: Keeps '?'
let url = new URL("http://ex.com/?");
console.log(`Before Delete: ${url.toString()}`)
url.searchParams.delete('NotPresent');
console.log(`After Delete: ${url.toString()}`)

This behavior is again inconsistent on Firefox's current behavior.
jsbin link

@TimothyGu
Copy link
Member Author

Indeed. And Edge apparently does not support the searchParams attribute...

@annevk
Copy link
Member

annevk commented Jul 4, 2017

Okay, I guess I'm good either way. Anyone up for filing browser bugs?

@stevenvachon
Copy link

stevenvachon commented Jul 5, 2017

I think it makes most sense to remove the "?" delimiter. By changing any param, the URL has been mutated and needn't preserve the delimiter. URLs with and without such are considered the same, anyway.

@TimothyGu Safari 10.1.1 doesn't have searchParams either.

@TimothyGu
Copy link
Member Author

@stevenvachon What do you think about the case in #6445 (comment)? Specifically, in Chrome, there is a case when no param is actually changed but the URL is still mutated. This just doesn't sit well with me. In my opinion the current spec behavior is still the most consistent.

@stevenvachon
Copy link

Deleting a non-existent param isn't technically a mutation, but it had the intent of such. We could always check before deleting.

@zimbabao
Copy link

zimbabao commented Jul 6, 2017

'?' should be present in URL only when there is at least one QP makes sense.
eg. it should be

new URL('http://w.co/?').toString() == new URL('http://w.co/').toString() 

@stevenvachon
Copy link

stevenvachon commented Jul 6, 2017

Also, when we parse http://host/? we do not get a { '': '' } value for searchParams.

@TimothyGu
Copy link
Member Author

'?' should be present in URL only when there is at least one QP makes sense.

This is a dangerous assumption. In fact it goes against what ALL implementations of URLs are doing today and the last few decades. If the spec is to be changed this way, this would mean that it would be impossible to visit a website like http://example.com/?, because the browser would always normalize it to http://example.com/. The cost would simply be too great.

Plus, we have to be careful to avoid slippery slope here. By this token, one could argue that a trailing # does not make sense either, and that would introduce breakages of its own.


Also, when we parse http://host/? we do not get a { '': '' } value for searchParams.

This is true. However, we should not conflate URL-level concepts with application/x-www-form-urlencoded-level concepts like searchParams. It is by tradition, not any standard, that the content in a URL's query generally follows application/x-www-form-urlencoded format. The current URL Standard as well as RFC 3986 does not impose any additional meaning or syntax to query. RFC 3986 describes a URL's query as:

The query component contains non-hierarchical data that, along with data in the path component (Section 3.3), serves to identify a resource within the scope of the URI's scheme and naming authority (if any).

The older RFC 2396 defines query as:

The query component is a string of information to be interpreted by the resource.

Based on these definitions, there is surely a difference between an empty query component and an undefined query component, a distinction reaffirmed by the URL Standard.

@stevenvachon
Copy link

stevenvachon commented Jul 6, 2017

Plus, we have to be careful to avoid slippery slope here. By this token, one could argue that a trailing # does not make sense either, and that would introduce breakages of its own.

I agree, but url.hash = '' will clear a trailing #. As should using URL#searchParams#delete() to remove the last param do so for a trailing ?. These actions are mutational

@annevk
Copy link
Member

annevk commented Jul 6, 2017

I don't see how base URL handling for # (which won't even work for normal URL objects as they don't have a stored base URL) relates to ?.

@stevenvachon
Copy link

@annevk I don't understand your comment. By "base URL", are you referring to new URL(url, base)? And I wasn't referring to a URL containing only #, but instead trailing on a full URL.

@annevk
Copy link
Member

annevk commented Jul 6, 2017

I meant that if url.href = '' does anything other than throwing (which it would with URL objects) it would be to resolve the input against a base URL (as <a>.href = '' would handle it for instance).

@stevenvachon
Copy link

Oops, I typo'ed. Comment edited.

@annevk
Copy link
Member

annevk commented Jul 6, 2017

I still don't understand what you're trying to say. Anyway, if you want to advocate some kind of change I recommend raising an issue against whatwg/url so we can consider it standalone. It shouldn't block this PR. My bad for taking in that direction to begin with.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Do you want me to file the bugs @TimothyGu?

@stevenvachon
Copy link

stevenvachon commented Jul 6, 2017

We could end up with a bigger issue if browsers need to be re-patched.

whatwg/url#332

@TimothyGu
Copy link
Member Author

@annevk Sure, go ahead.

@annevk annevk merged commit 3239799 into web-platform-tests:master Jul 7, 2017
@annevk
Copy link
Member

annevk commented Jul 7, 2017

@TimothyGu TimothyGu deleted the patch-1 branch July 7, 2017 11:19
annevk added a commit that referenced this pull request Jul 13, 2017
Changes outcome of test added in #6445.

Tests for whatwg/url#336.
annevk added a commit that referenced this pull request Jul 14, 2017
Changes outcome of test added in #6445.

Tests for whatwg/url#336.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants