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

Should URL constructors prepend ? when initializing query objects? #248

Closed
joyeecheung opened this issue Feb 14, 2017 · 2 comments
Closed

Comments

@joyeecheung
Copy link

This question comes from the Node.js implementation, refs: nodejs/node#11093

The URL constructor specifies:

Set result's query object to a new
{{URLSearchParams}} object using query, and then set that query object's
url object to result

And the URLSearchParams constructor specifies:

If init is given, is a string, and starts with "?", remove the first
code point from init.

But referencing the getter spec of url.search:

Return "?", followed by context object's url's query.

one can infer that the query of an url should not have the leading ?, this means:

  1. The URL constructor would pass an query with leading ? stripped to new URLSearchParams
  2. The new URLSearchParams would strip a leading ? again, in any, but the WPT tests expect ??a=b&c=d to be serialized as %3Fa=b&c=d

AFAICT the URL constructor should prepend a ? here, if we don't introduce a special way to notify the URLSearchParams constructor not to strip the leading ?:

diff --git a/url.bs b/url.bs
index 7988aa2..f1a9c9d 100644
--- a/url.bs
+++ b/url.bs
@@ -2648,7 +2648,8 @@ when invoked, must run these steps:
  <li><p>Set <var>result</var>'s <a for=URL>url</a> to <var>parsedURL</var>.

  <li><p>Set <var>result</var>'s <a for=URL>query object</a> to a <a for=URLSearchParams>new</a>
- {{URLSearchParams}} object using <var>query</var>, and then set that <a for=URL>query object</a>'s
+ {{URLSearchParams}} object using <var>query</var> prepended with the
+ leading <code>?</code>, and then set that <a for=URL>query object</a>'s
  <a for=URLSearchParams>url object</a> to <var>result</var>.

  <li><p>Return <var>result</var>.

Also the definition of url.query in 4.1. URL representation can be a little bit more explicit about the leading ?. I can put up a PR if this is confirmed.

@TimothyGu
Copy link
Member

"A new URLSearchParams object" doesn't invoke the constructor (which performs the stripping of ?), but instead goes to the specialized algorithm that skips the ?-stripping step. It is a bug in the Node.js implementation for using the constructor in such a fashion.

@joyeecheung
Copy link
Author

Thanks for the clarification, I missed out the concept-urlsearchparams-new part. I think this can be closed now.

TimothyGu added a commit to nodejs/node that referenced this issue Feb 17, 2017
PR-URL: #11372
Fixes: #11093
Ref: whatwg/url#248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 25, 2017
PR-URL: nodejs#11372
Fixes: nodejs#11093
Ref: whatwg/url#248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants