-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix vitest-pool-workers when nodejs_compat is enabled #11047
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
Fix vitest-pool-workers when nodejs_compat is enabled #11047
Conversation
🦋 Changeset detectedLatest commit: 134ddad The changes in this PR will be included in the next version bump. 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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
...itest-pool-workers-examples/basics-unit-integration-self/test/fetch-integration-self.test.ts
Show resolved
Hide resolved
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.
Do we really need a polyfill for node:url ?
eeaae54 to
522333d
Compare
|
@vicb I did a pass through and removed all unnecessary polyfills (turning on native features as needed). PTAL. |
| * @param enableFlag The flag that enables the feature. | ||
| * @param disableFlag The flag that disables the feature. | ||
| */ | ||
| function ensureFlag(flags: string[], enableFlag: string, disableFlag: string) { |
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.
As discussed offline, using a Set could simplify the handling of the flags in this PR.
I find it hard to figure out what that means:
ensureFlag(
runnerWorker.compatibilityFlags,
"enable_nodejs_tty_module",
"disable_nodejs_tty_module"
);ensure what flag (enable vs disable) is what (used vs not used)?
Maybe a wrapper class could help, constructed with an array, using a Set internally, having ensureUsed(flag) and ensureNotUsed(flag) and a flags getter returning an array
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.
I reworked the helper to make its purpose more clear: ab1f757
…the necessary native features are enabled
Occasionally I saw this failing during the global setup
d49d7d6 to
134ddad
Compare
Fixes #11028
Fixes #11029
Recent additions to native nodejs_compat modules (
node:consoleandnode:vm) broke the vitest-pool-workers as it relies upon specialized polyfills to ensure that the Vitest runner can operate. This broke because generally the vitest-pool-workers uses module fallback to provide the polyfill which no longer works when there is a native version available.The aim of the last bullet is for us to get an early warning if any changes to the runtime that are flagged behind a compat date will cause breakages to testing.