[workers-utils] Fix compat date resolution in workspaces#11910
[workers-utils] Fix compat date resolution in workspaces#11910ksawaneh wants to merge 2 commits intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 3981ff3 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 |
petebacondarwin
left a comment
There was a problem hiding this comment.
Nice little trick!
| // Note: this works because Wrangler depends on `miniflare` (and therefore `workerd`) | ||
| // in the monorepo. | ||
| const wranglerPackageJson = fileURLToPath( | ||
| new URL("../../wrangler/package.json", import.meta.url) | ||
| ); |
There was a problem hiding this comment.
I'm a bit worried that this is brittle due to relying upon the relative location of the wrangler package compared to this file. But I guess it is only in a test and if it changes the test will break and we'll know to change it.
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
@claude can you rebase and fix the conflicts on this PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
06af74e to
2514e08
Compare
|
Really sorry! I messed up this branch when rebasing and lost your commits. |
| const wranglerPackageJson = fileURLToPath( | ||
| new URL("../../wrangler/package.json", import.meta.url) | ||
| ); | ||
| const { date, source } = getLocalWorkerdCompatibilityDate({ | ||
| projectPath: wranglerPackageJson, | ||
| }); |
There was a problem hiding this comment.
🟡 Test passes file path instead of directory path to getLocalWorkerdCompatibilityDate
The test passes a file path (wranglerPackageJson) as projectPath, but getLocalWorkerdCompatibilityDate expects a directory path and appends package.json to it.
Click to expand
How the bug is triggered
The test at lines 17-22 does:
const wranglerPackageJson = fileURLToPath(
new URL("../../wrangler/package.json", import.meta.url)
);
const { date, source } = getLocalWorkerdCompatibilityDate({
projectPath: wranglerPackageJson,
});But getLocalWorkerdCompatibilityDate at packages/workers-utils/src/compatibility-date.ts:40-42 does:
const projectRequire = module.createRequire(
path.join(projectPath, "package.json")
);So if projectPath is /path/to/wrangler/package.json, then path.join(projectPath, "package.json") becomes /path/to/wrangler/package.json/package.json, which is an invalid path.
Actual vs Expected
- Actual: The test passes a file path, resulting in an invalid path like
/path/to/wrangler/package.json/package.json - Expected: The test should pass a directory path like
/path/to/wrangler
Impact
The test will fail because module resolution will fail with the invalid path, causing the function to fall back to the fallback date instead of successfully resolving workerd's compatibility date.
Recommendation: Change the test to pass a directory path instead of a file path:
const wranglerDir = fileURLToPath(
new URL("../../wrangler", import.meta.url)
);
const { date, source } = getLocalWorkerdCompatibilityDate({
projectPath: wranglerDir,
});Was this helpful? React with 👍 or 👎 to provide feedback.
|
if we go with the solution in #12387 I think the changes here will become irrelevant. I do think that we need to proceed with #12387 since that solution addresses the following issue: in pnpm projects that have a wrangler dependency but not a direct miniflare dependency, |
getLocalWorkerdCompatibilityDate()could fail in workspaces because it usedrequire.resolve("miniflare"), andminiflare’smainpoints atdist/(which may not exist until built). That caused the function to fall back even thoughworkerdis installed.This change:
miniflare/package.jsoninstead, so resolution works even beforeminiflareis built@cloudflare/workers-utilsCommands run:
pnpm checkpnpm test:ci --filter @cloudflare/workers-utilsPublic documentation
A picture of a cute animal (not mandatory, but encouraged)
/_/\
( o.o )