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

Merged
merged 2 commits into from
Jan 8, 2025

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: 76b27ac

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

This PR includes changesets to release 2 packages
Name Type
create-cloudflare Minor
@cloudflare/vitest-pool-workers 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/12669822903/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/12669822903/npm-package-wrangler-7388 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-workers-bindings-extension-7388 -O ./cloudflare-workers-bindings-extension.0.0.0-v5c9599258.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v5c9599258.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-create-cloudflare-7388 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-kv-asset-handler-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-miniflare-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-pages-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-unenv-preset-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-vitest-pool-workers-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-workers-editor-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-workers-shared-7388
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12669822903/npm-package-cloudflare-workflows-shared-7388

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


wrangler@3.100.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241230.0
workerd 1.20241230.0 1.20241230.0
workerd --version 1.20241230.0 2024-12-30

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 :/

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...

Comment on lines 1 to 6
---
"@cloudflare/vitest-pool-workers": minor
"create-cloudflare": minor
---

chore: add nodejs_compat by default in Vitest Pool Workers, and remove nodejs_compat flag from basic C3 templates
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would it better splitting this into 2 changesets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: 27f9840

@andyjessop andyjessop force-pushed the aj/vitest-node-compat branch from 7921997 to 27f9840 Compare January 8, 2025 10:28
…date @cloudflare/vitest-pool-workers-examples

chore: remove nodejs_compat from C3 templates

chore: add changeset

chore: use nodejs_compat_v2

chore: use nodejs_compat

chore: add node_compat if not v1 and not v2

chore: remove v2 globals

merge array pushes
@andyjessop andyjessop force-pushed the aj/vitest-node-compat branch from 27f9840 to 76b27ac Compare January 8, 2025 11:38
@andyjessop andyjessop merged commit 78c1649 into main Jan 8, 2025
31 checks passed
@andyjessop andyjessop deleted the aj/vitest-node-compat branch January 8, 2025 12:19
@workers-devprod workers-devprod mentioned this pull request Jan 8, 2025
penalosa pushed a commit that referenced this pull request Jan 10, 2025
#7388)

* chore: add nodejs_compat programmatically for Vitest Pool Workers, update @cloudflare/vitest-pool-workers-examples

chore: remove nodejs_compat from C3 templates

chore: add changeset

chore: use nodejs_compat_v2

chore: use nodejs_compat

chore: add node_compat if not v1 and not v2

chore: remove v2 globals

merge array pushes

* chore: add changesets
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants