-
Notifications
You must be signed in to change notification settings - Fork 564
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
Queues support #2064
Queues support #2064
Conversation
* Add CLI commands for Queues * Handle queue producers and consumers on wrangler publish * fixup! Handle queue producers and consumers on wrangler publish * Remove consumer commands from CLI * Update queue settings names Co-authored-by: Josh Wheeler <jwheeler@cloudflare.com>
* Support queues in wrangler dev local mode. remote mode is an error for now * fixup! Support queues in wrangler dev local mode. remote mode is an error for now * Move remote wrangler dev + queues check Co-authored-by: Josh Wheeler <jwheeler@cloudflare.com>
🦋 Changeset detectedLatest commit: 726d636 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/3394512414/npm-package-wrangler-2064 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/2064/npm-package-wrangler-2064 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/3394512414/npm-package-wrangler-2064 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.developers.workers.dev/runs/3394512414/npm-package-cloudflare-pages-shared-2064 |
Codecov Report
@@ Coverage Diff @@
## main #2064 +/- ##
==========================================
+ Coverage 73.16% 73.43% +0.26%
==========================================
Files 129 139 +10
Lines 8668 8965 +297
Branches 2270 2327 +57
==========================================
+ Hits 6342 6583 +241
- Misses 2326 2382 +56
|
…t they are needeed for development and early testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be exciting to get queues support landed! I had a couple comments, mainly around import export conventions
…mote mode. This is not yet supported during the beta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Anything marked as non-blocking (mainly the MSW stuff) doesn't need to be done by end of this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good! One lingering comment, but other than that lgtm
const queueErr = err as FetchError; | ||
if (queueErr.code === 100123) { | ||
// queue_not_found | ||
throw new Error( | ||
`Queue "${queue}" does not exist. To create it, run: wrangler queues create ${queue}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it in the first review, but this catch
block shouldn't swallow all non-handled errors—it should re-throw them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, that was the intent. Nice catch! Fixed.
Fwiw, queues need to be added the the D1 Shim file for it them to work with D1, see https://github.com/cloudflare/wrangler2/blob/main/packages/wrangler/templates/d1-beta-facade.js#L165-L173 |
I have no context on the shim, I will take @Skye-31's word on this @jbw1991 |
I think @geelen might have some more context on the D1 shim? Let's not merge this until that's figured out |
The shim calls the normal worker when D1 is used, but as it doesn't export a queue handler, the API throws the error of "no queue consumer exported" (or something similar) https://discord.com/channels/595317990191398933/1022981666744061972/1035613298860773387 for employees who can see it, @JacobMGEvans |
This is ready for review!
Notable changes: