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

Initial Queues Support #354

Merged
merged 13 commits into from Sep 7, 2022
Merged

Initial Queues Support #354

merged 13 commits into from Sep 7, 2022

Conversation

jbw1991
Copy link
Contributor

@jbw1991 jbw1991 commented Aug 31, 2022

Pulling over the commits from my private fork.

Also added two new commits to clean up some temporary docs, and adjust to the latest desired wrangler config for queues.

I'm hoping to get this cleaned up and merged so that I can begin integrating it into the wrangler queues branch.

@jbw1991 jbw1991 requested a review from mrbbot August 31, 2022 19:53
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! ✅ Thanks for breaking this up into smaller PRs previously. 🙂 Would you be able to fix the lint warnings (npm run lint) then I'm happy to merge. 👍

@jbw1991
Copy link
Contributor Author

jbw1991 commented Sep 1, 2022

Thanks @mrbbot! Just fixed those warnings.

@mrbbot
Copy link
Contributor

mrbbot commented Sep 1, 2022

Great, thank you! Just noticed there are lots of changes to the package-lock.json file. 😕 Which version of node/npm are you using? Also, would you like this to be merged and released to the public now? Or alongside the wrangler release?

@jbw1991
Copy link
Contributor Author

jbw1991 commented Sep 1, 2022

Whoops sorry, I probably did something silly to screw up the package-lock. I'm using node version 18.7.0 and npm version 8.15.0.

I'll have to get this merged/released before I can update the dependency in wrangler branch, right? Unless there is some way to do a custom miniflare release/version (and then update my wrangler branch to point at that?).

@jbw1991 jbw1991 changed the title [WIP] Initial Queues Support Initial Queues Support Sep 2, 2022
@jbw1991 jbw1991 requested a review from rts-rob September 2, 2022 15:35
@mrbbot
Copy link
Contributor

mrbbot commented Sep 3, 2022

We can publish this to miniflare@queues like miniflare@d1, but if you're OK with it, it would be better to publish this as an actual Miniflare release, so we don't have multiple diverging feature branches without some bug fixes applied. We can log a warning that queues support is experimental in QueuesPlugin like this:

if (this.#processedServiceBindings.length) {
ctx.log.warn(
"Service bindings are experimental. There may be breaking changes in the future."
);
}

@jbw1991
Copy link
Contributor Author

jbw1991 commented Sep 7, 2022

Thanks @mrbbot, l think it makes sense to do a regular miniflare release.

I believe I have fixed the package-lock issues, and added the warning log message you suggested.

I'm not sure what's up with the failing test on Windows, is that flakiness or did I break that test somehow?

@mrbbot
Copy link
Contributor

mrbbot commented Sep 7, 2022

This looks like a flaky test. I'll try investigate it quickly, but will probably just disable it on Windows. 👍
Aiming to get a release out today too. 🙂

@mrbbot
Copy link
Contributor

mrbbot commented Sep 7, 2022

Actually, just noticed the Miniflare logger wasn't being used in some places. Since I'd like to get this out today, I've gone ahead and made the change. Does this look ok to you?

@jbw1991
Copy link
Contributor Author

jbw1991 commented Sep 7, 2022

Yes, looks great, you are too kind! Feel free to merge (+squash if desired) whenever you are ready.

@mrbbot mrbbot merged commit 4c1bfdb into cloudflare:master Sep 7, 2022
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.

None yet

3 participants