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

Remove node_compat flag from C3 templates that use Vitest Pool Workers #7388

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

andyjessop
Copy link
Contributor

@andyjessop andyjessop commented Nov 29, 2024

Fixes #000.

Removes the need to add nodejs_compat flag in C3 templates just to use Vitest Pool Workers. Vitest Pool Workers will now inject the flag if it doesn't exist.

@andyjessop andyjessop added the e2e Run e2e tests on a PR label Nov 29, 2024
Copy link

changeset-bot bot commented Nov 29, 2024

🦋 Changeset detected

Latest commit: 7921997

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

This PR includes changesets to release 2 packages
Name Type
@cloudflare/vitest-pool-workers Minor
create-cloudflare Minor

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

Copy link
Contributor

github-actions bot commented Nov 29, 2024

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/12371250417/npm-package-wrangler-7388

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-wrangler-7388 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-workers-bindings-extension-7388 -O ./cloudflare-workers-bindings-extension.0.0.0-vf9826e061.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vf9826e061.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-create-cloudflare-7388 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-kv-asset-handler-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-miniflare-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-pages-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-vitest-pool-workers-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-workers-editor-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-workers-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12371250417/npm-package-cloudflare-workflows-shared-7388

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

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

@andyjessop andyjessop changed the title Aj/vitest node compat Remove node_compat flag from C3 templates that use Vitest Pool Workers Nov 29, 2024
Copy link
Contributor

@Cherry Cherry left a comment

Choose a reason for hiding this comment

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

This is incredibly interesting!

Won't this potentially cause issues where a project could work during tests (if it does nodejs_compat stuff) but then fail during deploy or in prod, creating a different and now hard to debug environment?

Perhaps simply doing nodejs_compat + no_nodejs_compat_v2 would make more sense, to reduce the amount of cruft added to workers by default (#7108), but avoid the environment differences?

@andyjessop
Copy link
Contributor Author

@Cherry thanks again for the thoughtful contribution. I think this issue is multi-layered. Your solution of nodejs_compat + no_nodejs_compat_v2 is a good technical solution but this isn't just about bundle size. One thing we're very concerned with is moving users towards web standards and away from reliance on Node.js APIs. Having it enabled by default is messaging that we would like to avoid.

Won't this potentially cause issues where a project could work during tests (if it does nodejs_compat stuff) but then fail during deploy or in prod, creating a different and now hard to debug environment?

Yes, this is a concern, for sure. We hope that we can minimise any potential problems by making it very difficult to deploy bad code. For example, we have fail safe checks at build and also at deploy time (where there is a remote check that the code is able to be run by workerd). Edge-cases are probably dynamic imports of node APIs and usage of globals like Buffer.

@DaniFoldi
Copy link
Contributor

I think unfortunately enabling nodejs_compat_v2 by default would break opentelemetry libs' platform detection - and possibly many others. Checking for globalThis.Buffer is still pretty common in libraries :/

@andyjessop andyjessop marked this pull request as ready for review December 17, 2024 10:06
@andyjessop andyjessop requested review from a team as code owners December 17, 2024 10:06
Comment on lines 374 to 375
runnerWorker.compatibilityFlags.push("nodejs_compat");
runnerWorker.compatibilityFlags.push("no_nodejs_compat_v2");
Copy link
Contributor

Choose a reason for hiding this comment

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

Pedantic NIT 😄

Suggested change
runnerWorker.compatibilityFlags.push("nodejs_compat");
runnerWorker.compatibilityFlags.push("no_nodejs_compat_v2");
runnerWorker.compatibilityFlags.push("nodejs_compat", "no_nodejs_compat_v2");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'm not really with it this morning...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

4 participants