[Stream] Add Stream binding local mode#13030
Conversation
🦋 Changeset detectedLatest commit: b2d629d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
APIError: Not Found: Not found |
|
@natewong1313 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
5a3954e to
83d3150
Compare
|
✅ All changesets look good |
|
lgtm! |
petebacondarwin
left a comment
There was a problem hiding this comment.
The miniflare plugin itself looks good - I have left a few comments but I think they are mostly just nits that you can choose to implement if you wish.
I would, though, like to see some changes to the tests - there are quite a few scenarios (e.g. non-happy path) that are not tested. But most importantly, I would like the following:
- can we add tests that check the plugin works on reloads (e.g.
setOptionsandpersistencechanges). - avoid using "real timers" in the tests (e.g.
await new Promise((r) => setTimeout(r, 5));) which slow down the tests and can also be potential causes of flakes under load. Other plugins implementenableFakeTimersand are able to use that approach to get the desired test coverage.
I am sure that OC could fix up these tests in no time. If you want help I could do probably do this?
Thanks Pete, I've added more tests with what you wanted and got rid of timers |
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
2313b84 to
b2d629d
Compare
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| options.stream.binding, | ||
| options.stream.remoteProxyConnectionString | ||
| ), | ||
| entrypoint: "StreamBinding", |
There was a problem hiding this comment.
@natewong1313 - I think there is a bug here. When in remote mode the binding proxy doesn't have a named entry point. It just has a default one. So this should be:
| entrypoint: "StreamBinding", | |
| entrypoint: options.stream.remoteProxyConnectionString ? undefined : "StreamBinding", |
There was a problem hiding this comment.
I was already making the same change for Flagship, so I made the same patch for this as well. Feel free to discard it if it doesn't look right!
|
Codeowners approval required for this PR:
Show detailed file reviewers |
This MR adds local mode support for the Stream binding. Its architected as follows
Note that this MR doesn't support creating direct upload URLs yet but I'll do that in a follow up MR
A picture of a cute animal (not mandatory, but encouraged)
