-
Notifications
You must be signed in to change notification settings - Fork 38
JS client: use native fetch when available. #181
Conversation
Yeah I tried that but the compiler gave me a finger :(
…On Fri, 10 Feb 2023 at 23:50, Doug Stevenson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/libsql-client/src/lib/driver/HttpDriver.ts
<#181 (comment)>:
> + if (typeof fetch === "function") {
+ response = await fetch(this.url, reqParams);
+ } else {
+ response = await crossFetch(this.url, reqParams);
+ }
It might be more elegant to express it like this instead?
const f = typeof fetch === "function" ? fetch : crossFetch
const response = await f(...)
—
Reply to this email directly, view it on GitHub
<#181 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWIT4HMBFEDVFHOJL43YGQDWW3A2LANCNFSM6AAAAAAUXWWHOY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Really? tsc gave me no issues with that. |
653cccd
to
d0d3e1a
Compare
@BearLemma this may overlap with #214 where I had to switch to axios because of the keep-alive mess. |
@MarinPostma Does axios work on CloudFlare? If it's not using native fetch behind the scenes, it might not work on CF at all. CF workers are one of our primary use case right now for this client library. |
uhh no it does not. So we abandon support for node? |
what a joke this language is... |
Not sure what you mean. This library already works on node, browsers, and react native via the cross-fetch library. This PR enables the use of native fetch if it's available (which cross-fetch inconveniently does not delegate to correctly) in order to support CF. |
I'm referring to #214. I need to find a way to make the version check portable. |
d0d3e1a
to
54ba8b4
Compare
bors merge |
Build succeeded:
|
Might be fixing #179
Needs testing though.