feat: browser pubsub

[alanshaw]

This PR enabled pubsub in the browser and paves the way for a switch to using fetch by default and allowing for cancelable requests via the use of AbortController.

It's mostly the work done in ipfs-shipyard/js-ipfs-http-client-lite#1 but adapted a bit for use here.

If approved, we can start work moving the other commands to use fetch. The work in https://github.com/ipfs-shipyard/js-ipfs-http-client-lite has proven the hard parts (uploading files) are all possible using the fetch API.

Since fetch is promise based, when moving the other commands it makes sense to just switch to async/await as per ipfs/js-ipfs#1670 (and callbackify instead of promisify).

Depends on:

• fix: reduce the number of concurrent requests in browser interface-js-ipfs-core#505

resolves #518
refs ipfs/js-ipfs#2093
resolves #932

[daviddias]

We are shipping a non versioned module to users? If ndjson is not interested in the contribution, please create a fork and maintain it.

[alanshaw]

Yes this needs to get resolved. I think it's not so much the PR is not welcome it's more time commitment. Max added me to concat-stream, perhaps I can also get admin here as well ;)

[daviddias]

Overall seems good to me, left some questions.

[hugomrdias]

This looks pretty awesome from a quick review on my phone 🥳 I would just like to suggest using https://github.com/sindresorhus/ky with ky-universal to remove the fetch helpers. I have been using this one for a while and it's a pretty good little wrapper around fetch and it's from sindre 💪.

[alanshaw]

ky looks good, but I don't think that it will remove the need for the helpers:

ok deals with when the response is not 2xx, which ky does do, but it also takes care of extracting Message from a JSON error response, which is custom to our API and something that ky will never do. So we'd still need a helper here, and I don't think it's worth switching for the sake of an if (!res.ok) statement. I'd rather keep it simple.

The other part where I thought it might be helpful is with the querystring, but we'd still need a helper like objectToQuery to extract the nully values out of the object we want to be stringified (since ky delegates to URLSearchParams and leaves ?foo=null intact). This helper just makes extracting options values easier because we don't have to do null checks everywhere.

Also, it's convenient to be able to specify repeated parameters as an array, e.g. { arg: [1, 2] } for ?arg=1&arg=2 but URLSearchParams just stringifies the array so you end up with ?arg=1,2 so we'd need objectToQuery for this anyway.

Unless I'm missing something I think these are the only two helpers that ky could help with but I don't think it would be worth the bundle size increase to only help with half the problem in each case.

Last thing worth mentioning is that in this PR, fetch can be provided as a config option which would help resolve issues like this one and is only really doable if there's a well specified API for fetch. Allowing people to pass their own ky would tie us to using it for the considerable, and ky doesn't allow you to pass a fetch impl to it.

[hugomrdias]

ok deals with when the response is not 2xx, which ky does do, but it also takes care of extracting Message from a JSON error response, which is custom to our API and something that ky will never do. So we'd still need a helper here, and I don't think it's worth switching for the sake of an if (!res.ok) statement. I'd rather keep it simple.

you can do whatever you need with https://github.com/sindresorhus/ky#hooks and if we create (https://github.com/sindresorhus/ky#kycreatedefaultoptions) a pre-configured instance with the options we need to use everywhere we only need one file for the http client. Right now we have four to support browser and node, ky-universal does that for us for free.

also ok gets the response as text and tries to json parse IMO an http client should consider the content-type.

The other part where I thought it might be helpful is with the querystring, but we'd still need a helper like objectToQuery to extract the nully values out of the object we want to be stringified (since ky delegates to URLSearchParams and leaves ?foo=null intact). This helper just makes extracting options values easier because we don't have to do null checks everywhere.

i don't know if i understand this null case here URLSearchParams implements the spec so this is custom behaviour. Still we can do whatever we need with https://github.com/sindresorhus/ky#hooks basically what you already did in querystring.browser.js and it will work in node and browser no need for querystring.js

Also, it's convenient to be able to specify repeated parameters as an array, e.g. { arg: [1, 2] } for ?arg=1&arg=2 but URLSearchParams just stringifies the array so you end up with ?arg=1,2 so we'd need objectToQuery for this anyway.

Again it may be convenient but it's not spec compliant URLSearchParams is.
We can still do this with hooks.

Unless I'm missing something I think these are the only two helpers that ky could help with but I don't think it would be worth the bundle size increase to only help with half the problem in each case.

ky weights 2.4kB so it's totally fine plus

• it's isomorphic with ky-universal, no need to have node and browser implementation
• Retries failed requests
• JSON option
• Timeout support
• URL prefix option
• Instances with custom defaults
• Hooks
• Download progress event handler. https://github.com/sindresorhus/ky#ondownloadprogress

Last thing worth mentioning is that in this PR, fetch can be provided as a config option which would help resolve issues like this one and is only really doable if there's a well specified API for fetch. Allowing people to pass their own ky would tie us to using it for the considerable, and ky doesn't allow you to pass a fetch impl to it.

i think @Gozala just wanted to use fetch instead of node's http, but we can still allow for custom http clients, ky can be used the same way as fetch https://github.com/sindresorhus/ky#kyinput-options.

Also ky api is very well specified https://github.com/sindresorhus/ky#api better than ok, we can allow the user to pass either a native fetch compliant or a ky compliant and swap the internal instance. There's also this PR sindresorhus/ky#153 that would allow us to override fetch inside ky.
This is also an argument to be spec compliant and use URLSearchParams or else any custom http client would need to support our custom querystring handling.

[alanshaw]

you can do whatever you need with https://github.com/sindresorhus/ky#hooks and if we create (https://github.com/sindresorhus/ky#kycreatedefaultoptions) a pre-configured instance with the options we need to use everywhere we only need one file for the http client. Right now we have four to support browser and node, ky-universal does that for us for free.

I don't understand what do we have 4 of? How would ky-universal resolve it?

also ok gets the response as text and tries to json parse IMO an http client should consider the content-type.

That's because our HTTP API sometimes returns text not JSON. We still need to extract the Message on error resposnes.

The other part where I thought it might be helpful is with the querystring, but we'd still need a helper like objectToQuery to extract the nully values out of the object we want to be stringified (since ky delegates to URLSearchParams and leaves ?foo=null intact). This helper just makes extracting options values easier because we don't have to do null checks everywhere.

i don't know if i understand this null case here URLSearchParams implements the spec so this is custom behaviour.

This is about converting options to query string values. If we're given an options object and want to extract a value from it but the user doesn't set it we would end up with a querystring like ?x=null for all the properties supported in options. We only want to send querystring values for options that the user explicitly sets.

e.g.

const options = {}
const qs = new URLSearchParams({ x: options.x })
console.log(qs.toString()) // ?x=undefined

We need to extract/convert querystring parameters from options objects because often they are kebab case in query string but camel case in JS to be idiomatic.

Still we can do whatever we need with https://github.com/sindresorhus/ky#hooks basically what you already did in querystring.browser.js and it will work in node and browser no need for querystring.js

We'd still need to do what we're doing in querystring.js/querystring.browser.js unless we want to write out in long hand the append to URLSearchParams for repeated querystring values.

Also, it's convenient to be able to specify repeated parameters as an array, e.g. { arg: [1, 2] } for ?arg=1&arg=2 but URLSearchParams just stringifies the array so you end up with ?arg=1,2 so we'd need objectToQuery for this anyway.

Again it may be convenient but it's not spec compliant URLSearchParams is.

Repeated querystring parameters is spec compliant. I'm talking about internally converting options to query string params. It's way less boilerplate to use array syntax for repeated values. This wouldn't be exposed to the user in any way.

ky weights 2.4kB so it's totally fine plus

I'm still not sold on the reason why we have to take this hit!

it's isomorphic with ky-universal, no need to have node and browser implementation

I don't understand - node-fetch is isomorphic - uses just fetch in the browser.

Also ky api is very well specified https://github.com/sindresorhus/ky#api better than ok,

No I meant fetch is well spec'd.

There's also this PR sindresorhus/ky#153 that would allow us to override fetch inside ky.

Okay. Hopefully that'll be merged soon!

This is also an argument to be spec compliant and use URLSearchParams or else any custom http client would need to support our custom querystring handling.

We don't have custom query string handling, but our API uses repeated parameters.

---

I think request retries and timeout support are good reasons to use ky. Everything else appears to be just different to what's in this PR.

[hugomrdias]

I don't understand what do we have 4 of? How would ky-universal resolve it?

We have fetch.js, configure.js, configure.browser.js, querystring.js and querystring.browser.js i think we can get way with just one if we setup a ky instance with a default config and hooks for the "custom" things we need.

Even without using ky we can merge querystring.js and querystring.browser.js because node supports URLSearchParams and we can use https://github.com/hugomrdias/iso-url to hide the isomorphic require.

If we need to have different default configs we can use https://github.com/ipfs/js-ipfs-utils/blob/master/src/env.js to conditional setup options without having two files to support browser and node.

That's because our HTTP API sometimes returns text not JSON. We still need to extract the Message on error resposnes.

humm and the api doesn't set the proper content-type headers ? or it returns as text but its actually a json string ?

ky weights 2.4kB so it's totally fine plus

I'm still not sold on the reason why we have to take this hit!

I think request retries and timeout support are good reasons to use ky. Everything else appears to be just different to what's in this PR.

yeah for me this is the most important part about using ky

• Retries failed requests
• JSON option
• Timeout support

and if we look at the code the majority of those 2.4kB is actually these features.

everything else in the above comments is only about "can we do the same in a simple manner using ky?" and the answer is yes we can using hooks.

[Gozala]

My 2 cents (without reading through the whole thread) would be try to optimize (ergonomics) for standard web APIs, so that fetch and URLSearchParams is all that is needed. Ideally one wouldn’t even need a client library

[alanshaw]

@hugomrdias another review please 🙏

[hugomrdias]

This looks very good!!
One last nitpick which should not block this PR:
With ky we don't need to always create URLSearchParams manually, we can just pass an object to the searchParams option for simples use cases.

https://github.com/sindresorhus/ky#searchparams