cloudflare-worker-local icon indicating copy to clipboard operation
cloudflare-worker-local copied to clipboard

feat: Web Streams Support

Open mrbbot opened this issue 3 years ago • 5 comments

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).

mrbbot avatar Dec 16 '20 12:12 mrbbot

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.

ThisIsMissEm avatar Dec 17 '20 18:12 ThisIsMissEm

@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)

gja avatar Dec 18 '20 06:12 gja

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.

ThisIsMissEm avatar Dec 18 '20 15:12 ThisIsMissEm

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.

matchs avatar Dec 18 '20 16:12 matchs

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

mrbbot avatar Dec 19 '20 12:12 mrbbot