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

feat: Web Streams Support #61

Closed
wants to merge 5 commits into from

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Dec 16, 2020

This PR adds Web Streams (provided by web-streams-polyfill) to workers' scope and adds ReadableStreamsupport to KV get, getWithMetadata and put functions.

It also switches to using the @titelmedia/node-fetch fork of node-fetch which uses Web Streams instead of NodeJS ones, allowing Web Streams to be used as response bodies. This also closes #39, as Response.redirect is implemented by this fork (without a default redirect status though, I've opened a PR).

Adds Web Streams API to worker context, and switches to the titelmedia fork of node-fetch which uses Web Streams instead of NodeJS ones. Also closes issue gja#39, as @titelmedia/node-fetch implements Response.redirect.
It looks like FormData is no longer supported. Trying to use it with `wrangler dev` throws an error.
@ThisIsMissEm
Copy link

Hi! I can see you've switched to @titelmedia/node-fetch and I'm not sure that it's actively maintained anymore by Titel Media — this was a project I worked on when I worked at Titel Media 2 years ago (June-Dec 2018), and whilst they likely did continuing using the tooling that it was built for (a local cloudflare workers runtime), I'm not sure if they actively maintain it.

You may be better maintaining your own fork / npm package.

@gja
Copy link
Owner

gja commented Dec 18, 2020

@ThisIsMissEm thanks for the warning. @titelmedia/node-fetch looks like it's pretty awesome!

@mrbbot I see in the other PR that the project is still maintained. However, it's massively divergent from the default node-fetch, so I'm not super keen on making this the default. Perhaps you could update this PR so that the version of fetch is passed in as an argument or something? We can update the documentation to demonstrate how to do this (into create-cloudflare-worker as well)

@ThisIsMissEm
Copy link

Emelia Smith thanks for the warning. @titelmedia/node-fetch looks like it's pretty awesome!

MrBBot I see in the other PR that the project is still maintained. However, it's massively divergent from the default node-fetch, so I'm not super keen on making this the default. Perhaps you could update this PR so that the version of fetch is passed in as an argument or something? We can update the documentation to demonstrate how to do this (into create-cloudflare-worker as well)

@gja the problem with that is then you don't actually conform to the Cloudflare Workers API, which is why that fork of node-fetch was created; node-fetch doesn't conform to the fetch API, and they don't intend to, so really they shouldn't be called "node-fetch" but more node-kinda-fetch. If you're trying to build a cloudflare worker-like environment, you'll definitely want to start from the @titelmedia/node-fetch fork.

It sounds like @matchs is actually maintaining the fork, I didn't know it's status, as it'd been years since I initially worked on it.

@matchs
Copy link

matchs commented Dec 18, 2020

Emelia Smith thanks for the warning. @titelmedia/node-fetch looks like it's pretty awesome!
MrBBot I see in the other PR that the project is still maintained. However, it's massively divergent from the default node-fetch, so I'm not super keen on making this the default. Perhaps you could update this PR so that the version of fetch is passed in as an argument or something? We can update the documentation to demonstrate how to do this (into create-cloudflare-worker as well)

@gja the problem with that is then you don't actually conform to the Cloudflare Workers API, which is why that fork of node-fetch was created; node-fetch doesn't conform to the fetch API, and they don't intend to, so really they shouldn't be called "node-fetch" but more node-kinda-fetch. If you're trying to build a cloudflare worker-like environment, you'll definitely want to start from the @titelmedia/node-fetch fork.

It sounds like @matchs is actually maintaining the fork, I didn't know it's status, as it'd been years since I initially worked on it.

@gja Just like @ThisIsMissEm said. The reason we forked and changed node-fetch was because, when we were internally implementing our own version of a local cloudflare-worker, we noticed that we couldn't actually reproduce how cloudflare-worker behaved because node-fetch wasn't compliant to the fetch API, specially on the type of body and streams.

I'm now curious to see how you're making it work with the default node-fetch (and would be glad to share a bit about our solution too - honestly, we should have made that open source years ago).

Just in case it's interesting for you, there's already other local cloudflare-workers implementation beyond our own that uses our fork as a starting point: https://github.com/dollarshaveclub/cloudworker.

@mrbbot
Copy link
Contributor Author

mrbbot commented Dec 19, 2020

Ooops! Didn't mean to push that to this branch.

@mrbbot mrbbot closed this Jan 4, 2024
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.

Response.redirect is not a function
4 participants