Skip to content

add workers assets to vitest integration#6835

Merged
emily-shen merged 11 commits into
mainfrom
emily/assets-vitest
Oct 7, 2024
Merged

add workers assets to vitest integration#6835
emily-shen merged 11 commits into
mainfrom
emily/assets-vitest

Conversation

@emily-shen

@emily-shen emily-shen commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

What this PR solves / how to test

Fixes WC-2771

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Changeset included
    • Changeset not necessary because:
  • Public documentation

@changeset-bot

changeset-bot Bot commented Sep 26, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 85c34dd

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

This PR includes changesets to release 4 packages
Name Type
miniflare Patch
@cloudflare/vitest-pool-workers Patch
@cloudflare/pages-shared Patch
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

Comment thread packages/miniflare/src/plugins/core/index.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This case should also cover the router worker switch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Might've misunderstood something, but this branch is for RPC entrypoints right, which shouldn't get assets...? To reflect prod behaviour.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh fair point, I had thought that you could define things as { name: "service-name" } with entrypoint being optional—didn't realise this was just for RPC

@emily-shen emily-shen Oct 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Turns out fetch on an RPC entrypoint can get assets ¯\_(ツ)_/¯

doesn't work in miniflare yet, so leave it for now with a todo comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think what you mean is that a fetch() handler on a WorkerEntryPoint type entry point (whether it is the default or a named entry-point) will check assets first (i.e. go via the Router Worker).

But actual RPC invocations (not to fetch()) such as await env.WORKER.foo() will not check assets first and will go directly to the User Worker.

Right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also other non-fetch() handlers such as scheduled and queue will not go via Router Worker

@emily-shen emily-shen Oct 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry that was imprecise - I meant fetch on a worker entrypoint should return assets.

Although currently SELF can only hit the default entrypoint (with fetch or RPC) right? So you still couldn't replicate prod behaviour for assets + fetch on a named entrypoints, since we decided importing the worker directly won't return assets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't you have service bindings to Worker + Assets in Vitest tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by a service binding sorry? As in another worker calling a "regular" fetch handler on a worker with assets? I hadn't thought about multiple workers in vitest with assets yet D:

What I was trying to say above was - from what I understand, SELF.fetch() will only ever hit the default entrypoint on the UW. If its on a named entrypoint, you need to import the worker and do worker.NAMED.fetch()

We also agreed previously that worker.fetch() shouldn't return assets because it makes sense that there is no RW if you're importing a worker directly.

So if you had a UW with a named entrypoint, you couldn't test that alongside assets (barring some janky workarounds with another worker in front). Feels like I'm getting into quite the edge case though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right. Sounds right.

@github-actions

github-actions Bot commented Sep 27, 2024

Copy link
Copy Markdown
Contributor

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-wrangler-6835

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

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6835/npm-package-wrangler-6835

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-wrangler-6835 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-create-cloudflare-6835 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-cloudflare-kv-asset-handler-6835
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-miniflare-6835
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-cloudflare-pages-shared-6835
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-cloudflare-vitest-pool-workers-6835
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-cloudflare-workers-editor-shared-6835
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11180182432/npm-package-cloudflare-workers-shared-6835

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.80.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240925.0
workerd 1.20240925.0 1.20240925.0
workerd --version 1.20240925.0 2024-09-25

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

Comment thread packages/wrangler/src/dev/miniflare.ts
@emily-shen
emily-shen marked this pull request as ready for review October 1, 2024 14:35
@emily-shen
emily-shen requested a review from a team as a code owner October 1, 2024 14:35
@emily-shen emily-shen added the ci:e2e Run wrangler + vite-plugin E2E tests on a pull request label Oct 3, 2024
@emily-shen
emily-shen merged commit 5c50949 into main Oct 7, 2024
@emily-shen
emily-shen deleted the emily/assets-vitest branch October 7, 2024 11:30
@workers-devprod workers-devprod mentioned this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:e2e Run wrangler + vite-plugin E2E tests on a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants