-
Notifications
You must be signed in to change notification settings - Fork 760
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
Conversation
🦋 Changeset detectedLatest commit: 76b27ac 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/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 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.
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...
.changeset/calm-lizards-tell.md
Outdated
--- | ||
"@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 |
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.
Nit: Would it better splitting this into 2 changesets?
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.
Added here: 27f9840
7921997
to
27f9840
Compare
…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
27f9840
to
76b27ac
Compare
#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
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.