-
Notifications
You must be signed in to change notification settings - Fork 747
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
base: main
Are you sure you want to change the base?
Conversation
…date @cloudflare/vitest-pool-workers-examples
🦋 Changeset detectedLatest commit: 7921997 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
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 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.
Please ensure constraints are pinned, and |
There was a problem hiding this 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?
@Cherry thanks again for the thoughtful contribution. I think this issue is multi-layered. Your solution of
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 |
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 :/ |
runnerWorker.compatibilityFlags.push("nodejs_compat"); | ||
runnerWorker.compatibilityFlags.push("no_nodejs_compat_v2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedantic NIT 😄
runnerWorker.compatibilityFlags.push("nodejs_compat"); | |
runnerWorker.compatibilityFlags.push("no_nodejs_compat_v2"); | |
runnerWorker.compatibilityFlags.push("nodejs_compat", "no_nodejs_compat_v2"); |
There was a problem hiding this comment.
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...
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.