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

Add fetch option #214

Merged
merged 3 commits into from
Jan 17, 2020
Merged

Add fetch option #214

merged 3 commits into from
Jan 17, 2020

Conversation

BrunoQuaresma
Copy link
Contributor

No description provided.

@BrunoQuaresma
Copy link
Contributor Author

Related to: #207

Copy link
Contributor

@danieltodonnell danieltodonnell left a comment

Choose a reason for hiding this comment

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

Code-wise, this looks great, but we should NOT merge this into the main code-base until we have some major users test it out first. We had issues previously with dynamically required libraries, and I only want to prerelease this until we do thorough testing.
NO MERGE YET.

@BrunoQuaresma
Copy link
Contributor Author

@danieltodonnell the dynamic requires was solved. We are already doing that on

? require('https')

src/Client.js Show resolved Hide resolved
@dihmeetree
Copy link

Code-wise, this looks great, but we should NOT merge this into the main code-base until we have some major users test it out first. We had issues previously with dynamically required libraries, and I only want to prerelease this until we do thorough testing.
NO MERGE YET.

I can do some debugging on this myself, and let you know if it works well or not!

@BrunoQuaresma
Copy link
Contributor Author

@lolcoolkat I would love that. Thanks!

@dihmeetree
Copy link

dihmeetree commented Jan 9, 2020

So I got it working inside my Cloudflare Worker by doing the following:

client: new faunadb.Client({
  secret,
  fetch: (req, init) => fetch(req, init),
})

Note: Passing fetch directly as a parameter gave me a TypeError: Illegal invocation error.

The only problem was that there seem's to be an issue with the following code, which gives me a TypeError: t.entries(...).forEach is not a function error.

responseHeadersAsObject(response),

Removing this line makes the FaunaDB client work fine again :) (However i'm sure that it definitely does something important :P)

@BrunoQuaresma
Copy link
Contributor Author

Thanks @lolcoolkat for the help on this.

  1. fetch: (req, init) => fetch(req, init) does not sound right.
  2. I'll check why the .entries().forEach is returning is not a function.

@mpavlinsky
Copy link

mpavlinsky commented Jan 9, 2020

I am also trying to get this working on cloudflare workers so I happen to have some answers:

  1. On cloudflare you can get the properly bound fetch with fetch.bind(globalThis)
  2. The fetch headers entries are an 'Iterator' not an array-like (https://developer.mozilla.org/en-US/docs/Web/API/Headers/entries) so I think the fallback iteration needs to change to something like
 var iter = responseHeaders.entries();
    var current;
    do {
      current = iter.next();
      if (current.value) {
        headers[current.value[0]] = current.value[1];
      }
    } while (!current.done);

(Or whatever the accepted way of iteration is in ES5, I assume we can't for ... of)

@dihmeetree
Copy link

dihmeetree commented Jan 9, 2020

Thank you @mpavlinsky !! Doing the following:

new faunadb.Client({
    secret,
    fetch: fetch.bind(globalThis),
})

seem's to do the trick! And for your # 2 that info is helpful. Thanks!

@BrunoQuaresma
Copy link
Contributor Author

BrunoQuaresma commented Jan 9, 2020

@mpavlinsky do you know if there is any doc related to the fetch() API used by CloudFlare Worker? I'm trying to understand why it is diff of the "standard" web API and if there is another differences to add in the tests to avoid future incompatibilities.

@mpavlinsky
Copy link

https://developers.cloudflare.com/workers/reference/apis/fetch/

Regarding the headers I think that is the behavior of the standard fetch. In my browser console fetch('https://google.com', {mode: 'no-cors'}).then(r => console.log(r.headers.entries().forEach)); returns undefined.

Use response.headers as Headers object like Web API specifies: https://developer.mozilla.org/en-US/docs/Web/API/Headers
@BrunoQuaresma
Copy link
Contributor Author

@lolcoolkat @mpavlinsky can you check if the last commit fixes the forEach() issue?

@dihmeetree
Copy link

@lolcoolkat @mpavlinsky can you check if the last commit fixes the forEach() issue?

Works perfectly! Thanks for the fix :)

test('uses custom fetch', async function() {
const fetch = jest.fn(() =>
Promise.resolve({
headers: new Set(),

Choose a reason for hiding this comment

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

I believe we want a test that exercises headers as an Iterator as is the case when using browser fetch

Copy link
Contributor Author

@BrunoQuaresma BrunoQuaresma Jan 10, 2020

Choose a reason for hiding this comment

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

The Iterator is returned by the entries(). The headers options are type of Headers[] which have a similar API with the Headers object. I used Set() to avoid extra mocking.

Headers ref: https://developer.mozilla.org/en-US/docs/Web/API/Headers/Headers

@BrunoQuaresma BrunoQuaresma self-assigned this Jan 17, 2020
@BrunoQuaresma BrunoQuaresma changed the base branch from master to 2.11.2-beta January 17, 2020 17:24
@BrunoQuaresma BrunoQuaresma merged commit a83acc1 into 2.11.2-beta Jan 17, 2020
@BrunoQuaresma BrunoQuaresma deleted the fe-236-add-fetch-option branch January 17, 2020 17:24
@BrunoQuaresma
Copy link
Contributor Author

Beta version published on https://www.npmjs.com/package/faunadb/v/2.11.2-beta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants