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

Feature: getMiniflareWaitUntil #345

Merged
merged 9 commits into from
Sep 7, 2022

Conversation

CraigglesO
Copy link
Contributor

Resolves #202

Adds getMiniflareWaitUntil(event: FetchEvent | ScheduledEvent): Promise<unknown>[]; to return an array of promises stored via event.waitUntill(...)

Example usage for both FetchEvent and ScheduledEvent can be found at packages/jest-environment-miniflare/test/fixtures/waitUntil.worker.spec.js

@CraigglesO CraigglesO changed the title Feature/wait until Feature: getMiniflareWaitUntil Aug 22, 2022
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.

Hey! 👋 Thanks for the PR! I've added some suggestions. I think we should implicitly Promise.all() for users, since they're very likely to do this themselves anyway.

packages/core/src/standards/event.ts Show resolved Hide resolved
Comment on lines 248 to 267
global.getMiniflareWaitUntil = (
event: FetchEvent | ScheduledEvent
): Promise<unknown>[] => {
return event[kWaitUntil];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

With the above change, you could replace this with a reference to the getWaitUntil function.

}

flush<WaitUntil extends any[] = unknown[]>(): Promise<WaitUntil> {
return Promise.all(this[kWaitUntil]) as Promise<WaitUntil>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I might be doing this wrong, but I get an error if I don't explain that any doesn't fall outside unknown.

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.

Hey! 👋 Added a couple more comments, but otherwise, looks good. 🙂

packages/jest-environment-miniflare/src/index.ts Outdated Show resolved Hide resolved
packages/jest-environment-miniflare/src/index.ts Outdated Show resolved Hide resolved
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.

One tiny thing, then I'll merge! ✅

packages/jest-environment-miniflare/src/index.ts Outdated Show resolved Hide resolved
@mrbbot
Copy link
Contributor

mrbbot commented Sep 7, 2022

Hey! 👋 I'm aiming to get a release out today and want to include this. I realised my changes with vitest-environment-miniflare clobbered with this change. I've fixed the conflict and will get this merged! 👍 Thanks again for doing this. 😃

@mrbbot mrbbot merged commit 379836d into cloudflare:master Sep 7, 2022
@CraigglesO
Copy link
Contributor Author

My fault for not finishing. Got really busy this week again. Just happy to see this go through!

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.

FetchEvent#waitUntil in Jest context
2 participants