-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: When converting a URL to a string, do not add a ?
if there are no query string parameters
#487
Conversation
4462ee6
to
314a4c2
Compare
pairs.clear().extend_pairs(self.list.iter()); | ||
match self.list.is_empty() { | ||
true => { | ||
url.url.set_query(None); |
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 the crux of the bug-fix.
Previously we used url.query_pairs_mut().clear()
however url.query_pairs_mut().clear();
is equivalent to url.set_query(Some(""))
, not url.set_query(None)
and we really wanted .set_query(None)
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.
Looks good to me, great find!
@@ -36,13 +36,26 @@ impl JSUrlSearchParams { | |||
match self.url_or_str { | |||
UrlOrString::Url(url) => { | |||
let url = unsafe { url.as_mut().unwrap() }; | |||
let mut pairs = url.url.query_pairs_mut(); | |||
pairs.clear().extend_pairs(self.list.iter()); | |||
match self.list.is_empty() { |
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.
Could you switch this to an if-then-else
, as it's matching a bool
value?
314a4c2
to
bc4d892
Compare
… no query string parameters fixes #484
bc4d892
to
8d6069f
Compare
Hi all -- is this live on fastly production services? |
Howdy, yes version 1.7.1 has this included 👍 https://github.com/fastly/js-compute-runtime/blob/main/CHANGELOG.md#171-2023-04-11 |
fixes #484