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

Use fetch instead of got in notion-client #312

Closed
wants to merge 2 commits into from

Conversation

remorses
Copy link
Contributor

@remorses remorses commented Jun 27, 2022

Description

Using fetch from undici or the native fetch implementation, this makes it possible to use notion-client in cloudflare workers or other environments other than node

Given that this is a breaking change (renamed gotOptions to fetchOptions) it could need a major version bump, tell me if you want me to change the version of the packages

Fix #160

@vercel
Copy link

vercel bot commented Jun 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-notion-x ✅ Ready (Inspect) Visit Preview Jun 27, 2022 at 0:58AM (UTC)
react-notion-x-minimal-demo ✅ Ready (Inspect) Visit Preview Jun 27, 2022 at 0:58AM (UTC)

@remorses remorses changed the title Sse fetch instead of got in notion-client Use fetch instead of got in notion-client Jun 27, 2022
@pbteja1998
Copy link
Contributor

Yes, even I use this cloudflare workers and i had to add a patch everytime to replace got with fetch.

@transitive-bullshit
Copy link
Member

Really great PR @remorses. Much appreciated 🙏

My main hesitancy here is that the notion API can be pretty flaky at times, and got includes some really nice, non-trivial retry logic by default. I'm worried that existing use cases would become less stable with this change.

Maybe we could allow for the user to pass in their own fetch implementation to the client constructor?

I'd still like got to be usable without any additional work on the user's part. Maybe we could move it to an optional peer dependency and try conditionally importing it if it's available? If not, then we fallback to the fetch logic in this PR?

@remorses
Copy link
Contributor Author

remorses commented Jul 3, 2022

What if we move the retry logic inside the fetch method? The user could then pass something like fetchOptions: {retry: 3}

@transitive-bullshit
Copy link
Member

I've finally had a chance to revisit this while trying to get notion-client to work with the new Vercel edge runtime.

The main issue I'm running into is that including node-fetch anywhere in the bundle (even conditionally imported) means that it doesn't work on non-node.js runtimes such as Vercel's edge runtime. I haven't tried with CF workers, but if it works there, then I'd guess the difference is Vercel's eager bundling optimizations.

Either way, I'd love to support this use case for notion-client, but am not sure of how to proceed aside from having the user pass in a fetch implementation which is kind of poor DX.

@transitive-bullshit
Copy link
Member

Resolved in #394

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.

Alternative to notion-client format?
3 participants