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

Spaces should be normalized as + in query strings #1113

Closed
1 task done
Richienb opened this issue Mar 11, 2020 · 19 comments · Fixed by #1051
Closed
1 task done

Spaces should be normalized as + in query strings #1113

Richienb opened this issue Mar 11, 2020 · 19 comments · Fixed by #1051
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@Richienb
Copy link
Contributor

Richienb commented Mar 11, 2020

What problem are you trying to solve?

Some web services like the VLC Web Interface use %20 to represent spaces in search params. However, Got automatically forces + with no way to override it even if put through query-string.

Describe the feature

The simplest solution would be to allow such overrides using the current syntax:

const got = require("got")
const queryString = require("query-string")

await got(`http://192.168.1.2/requests/status.json`), {
    port,
    password,
    searchParams: queryString.stringify({
        command,
        ...options,
    }).replace(/\+/, "%20")
})

The current workaround is to insert it directly into the url:

const got = require("got")
const queryString = require("query-string")

await got(`http://192.168.1.2/requests/status.json?${queryString.stringify({
    command,
    ...options,
}).replace(/\+/, "%20")}`), {
    port,
    password,
})

See: https://github.com/Richienb/vlc/blob/62f395cd471cf9cad0c540bad3aade8d2a7faa6f/src/index.ts#L169-L175

Checklist

  • I have read the documentation and made sure this feature doesn't already exist.
@szmarczak
Copy link
Collaborator

I cannot reproduce, can you create a RunKit example?

@Richienb
Copy link
Contributor Author

@szmarczak Copy and paste this into npm.runkit.com:

const got = require("got")
const queryString = require("query-string")

console.log((await got(`http://example.com`, {
    searchParams: queryString.stringify({
        a: "b c"
    }).replace(/\+/, "%20")
})).url)

It is supposed to log a URL with %20 as space.

@szmarczak
Copy link
Collaborator

You're using the searchparams option which converts the provided into a URLSearchParams instance. It converts %20 into +.

https://runkit.com/szmarczak/5e6aa4e35cec7b001b102009

@szmarczak szmarczak changed the title Allow %20 to be used instead of + for spaces Normalize spaces in search params Mar 12, 2020
@szmarczak szmarczak added documentation The issue will improve the docs enhancement This change will extend Got features ✭ help wanted ✭ labels Mar 12, 2020
@szmarczak
Copy link
Collaborator

@szmarczak
Copy link
Collaborator

@sindresorhus This is inconsistent:

new URL('https://example.com/?a=b c').toString()
"https://example.com/?a=b%20c"

new URLSearchParams('a=b c').toString()
"a=b+c"

@szmarczak
Copy link
Collaborator

const url = new URL('https://example.com/?a=b c');

url.search
"?a=b%20c"
url.searchParams.toString()
"a=b+c"

@szmarczak
Copy link
Collaborator

The WHATWG URL says that:

A URL object has an associated url (a URL) and query object (a URLSearchParams object).

@szmarczak
Copy link
Collaborator

But it also says:

A URL’s query is either null or an ASCII string holding data. It is initially null.

@szmarczak
Copy link
Collaborator

szmarczak commented Mar 12, 2020

url.searchParams.set('a', url.searchParams.get('a')) converts ?a=b%20c into ?a=b+c

@szmarczak
Copy link
Collaborator

Some web services like the VLC Web Interface use %20 to represent spaces in search params.

You should make an issue there instead. WHATWG URL is a standard and + should be accepted.

@szmarczak
Copy link
Collaborator

Modifying url.searchParams in any way (sort/append/delete) normalizes the query string properly.

@sindresorhus Maybe let's do url.searchParams.sort()?

@szmarczak szmarczak removed the documentation The issue will improve the docs label Mar 12, 2020
@szmarczak szmarczak changed the title Normalize spaces in search params Spaces should be normalized as + in query strings Mar 12, 2020
@sindresorhus
Copy link
Owner

Modifying url.searchParams in any way (sort/append/delete) normalizes the query string properly.

Since this works, it sounds like the original issue is more likely a Node.js bug in the URL implementation?

Maybe let's do url.searchParams.sort()?

Some people unfortunately depend on the order. I experienced that with https://github.com/sindresorhus/query-string

However, maybe we can just add a random obscure value and remove it again. Does that cause it to normalize? Like __GOT__INTERNAL__THING__.

@szmarczak
Copy link
Collaborator

Since this works, it sounds like the original issue is more likely a Node.js bug in the URL implementation?

It's in V8.

However, maybe we can just add a random obscure value and remove it again. Does that cause it to normalize? Like GOT__INTERNAL__THING.

image

@sindresorhus
Copy link
Owner

It's in V8.

How is it a bug in V8?


I would prefer workaround number 3.

@szmarczak
Copy link
Collaborator

How is it a bug in V8?

The bug also appears in browsers...

@sindresorhus
Copy link
Owner

@szmarczak That doesn't necessarily mean it's a bug in V8 though. Both the browser and Node.js URL implementation is based on the same spec and might have the same implementation bug or spec bug.

@TimothyGu
Copy link

There's a spec bug for the inconsistency between search and URLSearchParams: whatwg/url#18 (comment)

The implementation for URL and URLSearchParams is not in V8. Node.js and Chromium have separate implementations, with Node.js much closer to the WHATWG URL Standard than Chromium.

szmarczak added a commit to szmarczak/got that referenced this issue Apr 1, 2020
@szmarczak szmarczak mentioned this issue Apr 1, 2020
18 tasks
@tusbar
Copy link

tusbar commented Apr 3, 2020

I just observed the same issue – it’s not uncommon for web servers to not accept +: Amazon’s MWS API only supports %20 in form body and query string.

Workaround’s the same as the OP: cannot use searchParams nor form options.

@szmarczak
Copy link
Collaborator

Amazon’s MWS API only supports %20 in form body and query string

We do everything according to the spec, if they don't follow it - email them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants