-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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, querystring: web platform tests broken by WHATWG URL implementation #11093
Comments
I'm starting to wonder if the whatwg url implementation should just have its own querystring implementation ... |
Yeah I agree, when it is the spec behavior that is breaking(and breaking big), it's impossible to make a choice. I remember @TimothyGu mentioned he had implemented another one in C++? |
That was what I was pushing for. See #10967 (comment) and #10821. |
Yes, my intent all along was to have a separate parsing algorithm for querystring but just hadn't managed to get to it. |
Actually, reopening this since only first issue of leading |
We can probably close this in favor of #10821? |
@joyeecheung, I'd be ok with that |
Closed in favor of #10821. |
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>
Version: 90c2ac7
Broken tests discovered in #11079 that don't have tracking issues:
Leading
??
In upstream urltestdata.json:
Expected
searchParams.toString()
to be%3Fa=b&c=d
, gota=b&c=d
(the second?
is ignored).Also presents in upstream url-constructor.html
Note: this only appears when we parse through
URL
Space should be escaped as
+
In upstream url-constructor.html
In urlsearchparams-stringifier.html
Currently it's escaped as
%20
by thequerystring
module. I suspect fixing this inquerystring
might be too breaking though.The text was updated successfully, but these errors were encountered: