cloudflare / workers-sdk

โ›…๏ธ Home to Wrangler, the CLI for Cloudflare Workersยฎ
https://developers.cloudflare.com/workers/
Apache License 2.0
2.65k stars 694 forks source link

๐Ÿš€ Feature Request: support `wrangler.toml` configuration and TypeScript entrypoints for auxiliary Workers #5264

Open mattrubin opened 7 months ago

mattrubin commented 7 months ago

(Note: this issue was updated to track support for wrangler.toml configuration and TypeScript entrypoints for auxiliary Workers, see https://github.com/cloudflare/workers-sdk/issues/5264#issuecomment-2008044798)

I would like to test my project with @cloudflare/vitest-pool-workers, to ensure it is able to run on workerd. Unfortunately, the fact that @cloudflare/vitest-pool-workers requires the nodejs_compat compatibility flag introduces the possibility of code that passes the tests but fails to build via wrangler.

Versions used:

  • wrangler 3.34.2
  • @cloudflare/vitest-pool-workers 0.1.2

The code in question uses crypto.subtle.verify from the Web Crypto API, which does not require Node.js compatibility. However, the line import crypto from 'node:crypto'; was mistakenly added to the file.

This import is valid when the tests are run with the mandatory nodejs_compat, and so the test suite passes without issue.

When I run the worker (which does not have nodejs_compat in its wrangler.toml) via wrangler dev, it fails to start with:

โ–ฒ [WARNING] The package "node:crypto" wasn't found on the file system but is built into node.

and

โœ˜ [ERROR] service core:user:worker: Uncaught Error: No such module "node:crypto".

I am curious what the recommended approach to this problem might be. I can think of several possibilities: 1) Add the nodejs_compat compatibility flag to all my workers โ€“ even though it is not necessary โ€“ just to ensure the prod and test runtimes are the same 2) Add a test that uses unstable_dev to build and run the worker via Wrangler (This might require a separate test suite with a different vitest config, since I don't think it is possible to use unstable_dev from inside @cloudflare/vitest-pool-workers.) 3) Add wrangler deploy --dry-run to my npm test script, to check the result of the actual build process in parallel with running the test suite 4) Leave this all as-is, accept the risk of code that passes the tests but fails to build, and wait for the nodejs_compat requirement to (hopefully) be lifted in the future

I would prefer to avoid option 1, as it adds unnecessary runtime dependencies (and potential complexity) to my workers. Options 2 and 3 are both possible, and option 3 in particular is not an unreasonable change to make, but this would only help with testing workers โ€“ testing non-worker packages that are meant to be depended on by workers would require wrapping the code in a test-only worker to make use of Wrangler's build process. Option 4 would feel palatable only if removing nodejs_compat is an anticipated goal of the @cloudflare/vitest-pool-workers project.

I'd appreciate any guidance or suggestions from the project's developers. Thank you for all of the work that has already gone into the new Vitest integration!

mrbbot commented 7 months ago

Hey! ๐Ÿ‘‹ For the time being, I'd recommend you either:

I think it's going to be tricky to remove the nodejs_compat flag requirement for the test runner Worker any time soon, as Vitest was initially designed for Node.js and relies on quite a few of these APIs.

mattrubin commented 7 months ago

Thanks for the quick reply! I'll give your second suggestion a try.

mattrubin commented 7 months ago

The suggested approach โ€“ building the worker in a global setup script, then running it as an auxiliary worker during testing โ€“ worked well as a solution for catching the sort of runtime dependency resolution issue described above. ๐Ÿ‘

However, the primary limitation of this approach is that there seems to be no mechanism for mocking outbound fetch() requests made by an auxiliary worker. This means that the actual testing of my worker's functionality needs to still be done via unit testing and SELF-based integration testing. In this case, the auxiliary worker testing exists simply to answer the question "will my worker run in a production-like environment?".

@mrbbot This and other limitations of auxiliary workers are fairly well documented, and the docs do acknowledge the developer experience shortcomings. That said, would you welcome new feature request/improvement GitHub issues to track these areas of potential improvement, or do you feel that they are already tracked elsewhere (or are unfixable)?

mrbbot commented 7 months ago

there seems to be no mechanism for mocking outbound fetch() requests made by an auxiliary worker

This is possible using the Miniflare outboundService or fetchMock options. See here for an example:

https://github.com/cloudflare/workers-sdk/blob/0e149d79b2fb834c69637ae4371bd4d20a7a1057/fixtures/vitest-pool-workers-examples/multiple-workers/vitest.config.ts#L86-L87

See https://github.com/cloudflare/workers-sdk/tree/main/packages/miniflare#interface-workeroptions for documentation. To create a MockAgent for the fetchMock option, import and call the createFetchMock() function from the miniflare package.


I think it's going to be tricky for us to provide the same developer experience for auxiliary workers. Not using Vite for module resolution and transformation makes them good for as-close-to-production integration tests, but also makes it hard to provide things like automatic test re-runs. It's possible we could improve the experience with support for TypeScript entrypoints, but I don't think it's possible to get the same DX without sacrificing test fidelity.

mattrubin commented 7 months ago

there seems to be no mechanism for mocking outbound fetch() requests made by an auxiliary worker

This is possible using the Miniflare outboundService or fetchMock options. See here for an example:

This is fantastic, thanks! I had interpreted too broadly the documentation's statement that auxiliary workers are "not affected by global mocks defined in your tests" and thought that fetch mocking was not possible.


I think it's going to be tricky for us to provide the same developer experience for auxiliary workers. Not using Vite for module resolution and transformation makes them good for as-close-to-production integration tests, but also makes it hard to provide things like automatic test re-runs. It's possible we could improve the experience with support for TypeScript entrypoints, but I don't think it's possible to get the same DX without sacrificing test fidelity.

Understood.

Personally, automatic test re-runs are less of a priority for integration tests. At the top of my DX wishlist would be:

โ€ฆbut I also understand if the effort involved does not make these changes worth it for the somewhat-smaller DX improvements.

mrbbot commented 7 months ago

Ok, I'm going to repurpose this issue to track adding support for wrangler.toml and TypeScript entrypoints to auxiliary Workers. I agree these would be good DX improvements. I'll update the title, and add a note to your initial description to capture this. ๐Ÿ‘