fix: ensure Vitest Pool Workers doesn't error with nodejs_compat_v2 flag #7278
Conversation
🦋 Changeset detectedLatest commit: 648b797 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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/11894006724/npm-package-wrangler-7278You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7278/npm-package-wrangler-7278Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-wrangler-7278 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-create-cloudflare-7278 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-kv-asset-handler-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-miniflare-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-pages-shared-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-vitest-pool-workers-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-workers-editor-shared-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-workers-shared-7278npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11894006724/npm-package-cloudflare-workflows-shared-7278Note 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.
I think flag-assertions is too generic. Can it be called compatibility-flag-assertions etc?
| const assertions = [ | ||
| () => | ||
| flagAssertions.assertDisableFlagNotPresent("export_commonjs_namespace"), | ||
| () => | ||
| flagAssertions.assertEnableFlagOrCompatibilityDate( | ||
| "export_commonjs_default", | ||
| { | ||
| compatibilityDate: runnerWorker.compatibilityDate, | ||
| defaultOnDate: "2022-10-31", | ||
| } | ||
| ), | ||
| () => | ||
| flagAssertions.assertUnionOfEnableFlags([ | ||
| "nodejs_compat", | ||
| "nodejs_compat_v2", | ||
| ]), | ||
| ]; | ||
|
|
||
| for (const assertion of assertions) { | ||
| const result = assertion(); | ||
| if (!result.isValid) { | ||
| throw new Error(result.errorMessage); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Miniflare already exposes a function for working out what the current node_compat mode we are in (it is not trivial due to the negation flags).
See
I think that isFlagPresent() is actually too simple to cover the cases.
I would argue that we should just use the getNodeCompat() function from Miniflare, except that I see that we also need this other export_commonjs_namespace compat flag, which potentially also has complexity due to compat date default and negation.
I propose that if we are going to have this logic here, we just create a function called isEnabled(enableFlag: string, disableFlag: string, defaultOnDate?: string), then you could just do:
if (!compatFlagAssertions.isEnabled("export_commonjs_default", "export_commonjs_namespace", "2022-10-31")) {
throw new Error("export_commandjs_default must be enabled - either explicitly or by compat date being after 2022-10-31");
}
if (
!compatFlagAssertions.isEnabled("nodejs_compat", "no_nodejs_compat") &&
!compatFlagAssertions.isEnabled("nodejs_compat_v2", "no_nodejs_compat_v2")
) {
throw new Error("Node.js compatibility must be enabled - by adding either `nodejs_compat` or `nodejs_compat_v2` compat flag");
}There was a problem hiding this comment.
At the moment, we also have messaging depending on the options path, the wrangler path, and the relative project path. Are you suggesting we just drop that for a simpler set of messages? Most of the code in this class is dealing with messaging, so being liberated from that would simplify things significantly.
There was a problem hiding this comment.
OK looking at the code again, I think we could just reuse most of the logic in assertCompatibiltyFlagEnabled() but update it so that enableFlag: string becomes enableFlags: string[] and similarly with disableFlag: string becoming disableFlags: string[].
The logic just needs a bit of tweaking to ensure that none of the disableFlags are in there and either at least one of the enableFlags is there or the compatibility date is new enough.
So then still throwing inside that function.
There was a problem hiding this comment.
And if you still want to move it out into a class that is cool too!
There was a problem hiding this comment.
I've updated this to consolidate the assertDisableFlagNotPresent and assertEnableFlagOrCompatibilityDate methods, but will wait for discussion on the messaging before proceeding further with the refactor.
There was a problem hiding this comment.
Sorry Pete, I didn't see your comment before pushing my latest update.
Fixes #7156.
In Vitest Pool Workers, we assert the presence of the
nodejs_compatflag, but it is also possible to use thenodejs_compat_v2flag, so we have to account for this possibility when asserting the validity of the config file.This PR creates a new class to handle the validation, simplifying the logic as well as the testing. Some small improvements alongside adding the
nodejs_compat_v2functionality:miniflare)testblock to allow for simpler debugging.