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.

Sign up for GitHub

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

Jump to bottom

fix: url.search is a string so do not set it as a URLSearchParams #103

Closed
wants to merge 1 commit into from
Closed

fix: url.search is a string so do not set it as a URLSearchParams #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ class HTTP {

if (searchParams) {
if (typeof transformSearchParams === 'function') {
// @ts-ignore
url.search = transformSearchParams(new URLSearchParams(opts.searchParams))
} else if (searchParams instanceof URLSearchParams) {
url.search = searchParams.toString()
} else {
// @ts-ignore
url.search = new URLSearchParams(opts.searchParams)
url.search = new URLSearchParams(searchParams).toString()
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface HTTPOptions extends FetchOptions {
/**
* Transform search params
*/
transformSearchParams?: (params: URLSearchParams) => URLSearchParams
transformSearchParams?: (params: URLSearchParams) => string
Copy link
Member

Choose a reason for hiding this comment

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

its safer to work with a URLSearchParams instance, so why return a string here ? we can call .toString internally

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think that URLSearchParams is a better representation to work with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to work around the way go-IPFS expects binary data to be URL encoded (see ipfs/kubo#7939 for context).

In a nutshell 0xFE (decimal 254) needs to be encoded as %FE and not %C3%BE, so I was doing the encoding myself but if you have to pass back a URLSearchParams instance, it'll re-encode the data, which makes it garbage on the server when it's decoded.

I don't think it's necessarily the way to go, since now everyone has to use a custom encoder/decoder - instead we should be able to pass base64 encoded data to the HTTP API if it has to go on the query string, but the fact that we have to @ts-ignore the assignment here seemed a bit weird.

URL.search is a string, and the return type of transformSearchParams gets turned into a string when it is assigned to URL.search so why not let the user return a string?

Copy link
Member

Choose a reason for hiding this comment

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

im good with the internal change to assign strings to search just not the return type change.
I know transformSearchParams is basically just for our use but keeping the return type URLSearchParams will be at least a reminder to not manually encoded querystrings.

Copy link
Member Author

Choose a reason for hiding this comment

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

keeping the return type URLSearchParams will be at least a reminder to not manually encoded querystrings.

The thing is, I needed to manually encode a query string.

Copy link
Member

Choose a reason for hiding this comment

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

ah lol, if we can avoid that lets ship this

/**
* When iterating the response body, transform each chunk with this function.
*/
Expand Down
19 changes: 19 additions & 0 deletions test/http.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,23 @@ describe('http', function () {
const rsp = await req.arrayBuffer()
expect(uint8ArrayEquals(new Uint8Array(rsp), buf)).to.be.true()
})

it('allows transforming search params', async function () {
const res = await HTTP.post('echo/query', {
base: ECHO_SERVER,
body: 'hello world',
searchParams: new URLSearchParams({
foo: 'bar'
}),
transformSearchParams: (params) => {
expect(params.get('foo')).to.equal('bar')

return 'baz=qux'
}
})

const query = JSON.parse(await res.text())

expect(query).to.have.property('baz', 'qux')
})
})