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

feat: trigger scheduled events from fetch events for testing #1722

Closed
wants to merge 4 commits into from

Conversation

cameron-robey
Copy link
Contributor

@cameron-robey cameron-robey commented Aug 23, 2022

  • Implements triggering scheduled events from a fetch event for testing
  • Triggers by a fetch event at the URL: /___scheduled
  • Implemented for both module workers and service workers
  • Lets through all other fetch events so does not modify any other behaviour of workers

Currently only switches on with an environment variable FETCH_TRIGGER_CRON=true - we need to think about how we trigger this.
The advantage of the way that Miniflare implements this is that /cdn-cgi/mf is never going to accessible to a worker anyway, and (probably) never will be used internally so there is no chance of it overwriting an actual route.
The need therefore is to find a url that will pretty much never be used so that we can use it for testing cron workers - suggested here is /___scheduled. If it is never used then we could enable this functionality all the time for wrangler dev, rather than needing to enable with a command line flag.

Closes: #570

To do before release:

A few other notes:

  • miniflare implements this by using the route /cdn-cgi/mf/scheduled which we cannot access as /cdn-cgi is not accessible when running on the edge. Once this has been landed we should migrate miniflare to using the same endpoint as here.
  • With the open source runtime miniflare may no longer be able to access /cdn-cgi so it may need to migrate anyway

@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2022

🦋 Changeset detected

Latest commit: e93a994

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2022

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/2912204504/npm-package-wrangler-1722

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1722/npm-package-wrangler-1722

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2912204504/npm-package-wrangler-1722 dev path/to/script.js

@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #1722 (e93a994) into main (9e632cd) will decrease coverage by 1.46%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1722      +/-   ##
==========================================
- Coverage   82.00%   80.53%   -1.47%     
==========================================
  Files          89       90       +1     
  Lines        5884     6135     +251     
  Branches     1509     1586      +77     
==========================================
+ Hits         4825     4941     +116     
- Misses       1059     1194     +135     
Impacted Files Coverage Δ
packages/wrangler/src/bundle.ts 63.56% <27.77%> (-5.81%) ⬇️
packages/wrangler/src/zones.ts 67.69% <0.00%> (-25.65%) ⬇️
...ngler/src/pages/functions/routes-transformation.ts 80.39% <0.00%> (-19.61%) ⬇️
packages/wrangler/src/pages/publish.tsx 43.79% <0.00%> (-8.49%) ⬇️
packages/wrangler/src/tail/printing.ts 85.71% <0.00%> (-1.79%) ⬇️
packages/wrangler/src/pages/dev.tsx 22.47% <0.00%> (-1.51%) ⬇️
packages/wrangler/src/cfetch/internal.ts 8.00% <0.00%> (-1.46%) ⬇️
...ages/wrangler/src/__tests__/helpers/mock-cfetch.ts 95.00% <0.00%> (-0.72%) ⬇️
packages/wrangler/src/pages/deployments.tsx 79.54% <0.00%> (-0.46%) ⬇️
packages/wrangler/src/index.tsx 89.04% <0.00%> (-0.06%) ⬇️
... and 18 more

@cameron-robey cameron-robey marked this pull request as ready for review August 23, 2022 13:24
@cameron-robey cameron-robey changed the title wip: basic support to trigger scheduled from fetch events feat: trigger scheduled events from fetch events for testing Aug 23, 2022
Comment on lines +8 to +16
export let swAddEventListener = (event, listener) => {
if (event === "fetch") {
globalThis.__inner_sw_fetch_listeners__.push(listener);
} else if (event === "scheduled") {
globalThis.__inner_sw_scheduled_listeners__.push(listener);
} else {
globalThis.addEventListener(event, listener);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Workers global scopes are instances of ServiceWorkerGlobalScope which inherits from EventTarget. This provides the addEventListener method, along with removeEventListener and dispatchEvent. There are quite a few subtleties when using EventTargets. For example, listener may be a function, or an object of the form { handleEvent(event) { ... } }. Additionally, calling FetchEvent#respondWith() should implicitly call Event#stopImmediatePropagation(), meaning further event listeners aren't called. I'll talk to offline about a slightly different approach that means you don't have to implement all of this logic yourself.

@ObsidianMinor
Copy link
Contributor

With the open source runtime miniflare may no longer be able to access /cdn-cgi so it may need to migrate anyway

Nothing in the runtime prevents access to cdn-cgi, it's another edge service that runs before the runtime that explicitly handles it.

@shyim
Copy link

shyim commented Sep 3, 2022

How would be the URL when the entry point of a worker is not just / like /api 🤔

@cameron-robey
Copy link
Contributor Author

Closing in favour of #1815

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.

Support cron triggers using preview and dev
4 participants