Add minimum and maximum version checks for frameworks during auto-configuration#13050
Add minimum and maximum version checks for frameworks during auto-configuration#13050dario-piotrowicz wants to merge 5 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 98d490a 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 |
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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: |
|
Review posted on PR #13050. The main finding is a logic bug where |
dario-piotrowicz
force-pushed
the
dario/DEVX-2525/autoconfig-min-max-versions
branch
5 times, most recently
from
0511cd7 to
9cdad82
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dario-piotrowicz
force-pushed
the
dario/DEVX-2525/autoconfig-min-max-versions
branch
2 times, most recently
from
b74a618 to
a69f70b
Compare
dario-piotrowicz
force-pushed
the
dario/DEVX-2525/autoconfig-min-max-versions
branch
from
a69f70b to
84dd3a2
Compare
| if ( | ||
| semiver(frameworkVersion, frameworkPackageInfo.maximumKnownMajorVersion) > | ||
| 0 | ||
| ) { | ||
| logger.warn( | ||
| `The version of ${this.name} used in the project (${JSON.stringify( | ||
| frameworkVersion | ||
| )}) is not officially known / supported by the Wrangler automatic configuration. So the operation might not complete successfully. Please report this to workers-sdk` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 maximumKnownMajorVersion comparison emits spurious warnings for versions with non-zero patch
The semiver(frameworkVersion, frameworkPackageInfo.maximumKnownMajorVersion) comparison at framework-class.ts:76 incorrectly warns for versions within the known major that have a non-zero patch component. When maximumKnownMajorVersion is "4", semiver splits it into ["4"]. The minor comparison (fn(a[1], undefined)) returns 0 due to JS undefined comparison semantics. But the patch comparison uses a || 0 fallback: fn(a[2] || 0, b[2] || 0) where b[2] is "" → "" || 0 = 0. So any non-zero patch triggers a false positive: semiver("4.3.2", "4") returns 1 (warning), while semiver("4.5.0", "4") returns 0 (no warning). In practice, versions like "6.0.3" of Astro (with maximumKnownMajorVersion: "6") would display a misleading warning that the version is "not officially supported". The test at line 113 ("4.3.2" expecting no warning) would also fail for the same reason.
Suggested fix approach
Compare only the major version number instead of using semiver with a partial version string:
const installedMajor = parseInt(frameworkVersion.split(".")[0], 10);
const maxKnownMajor = parseInt(frameworkPackageInfo.maximumKnownMajorVersion, 10);
if (installedMajor > maxKnownMajor) { logger.warn(...); }
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
semiver("4.5.0", "4") doesn't return 1 🤔
There was a problem hiding this comment.
I've added a test to ensure that we don't warn: b073efd
| projectPath: string | ||
| ): string | undefined { | ||
| try { | ||
| // require.resolve on package.json is more reliable so let's try that first |
There was a problem hiding this comment.
Why is this more reliable?
There was a problem hiding this comment.
because the package.json could not have a resolve export causing require.resolve to throw ERR_PACKAGE_PATH_NOT_EXPORTED this is actually the case for nuxt
however require.resolve on package.json is also not a silver bullet since this only works if the package.json is present in the published package, so I think the best is to try both
There was a problem hiding this comment.
PS: I'm happy to expand on the code comment if that'd help 🙂
There was a problem hiding this comment.
I don't think it is possible to have a published package that does not contain package.json??
There was a problem hiding this comment.
mh... no sorry I think you're right... but there was something regarding the package.json that I saw a while back... like require.resolve not being able to see package.jsons under certain circumstances 🤔 , I'll try to find what that was
There was a problem hiding this comment.
Sorry, that was totally my bad, it seems like require.resolve only detects package.jsons when they are exported: nodejs/node#33460
Co-authored-by: Somhairle MacLeòid <samuel@macleod.space>
4 participants
Fixes https://jira.cfdata.org/browse/DEVX-2525
When Wrangler automatically configures a project, it now validates the installed version of the detected framework before proceeding:
A picture of a cute animal (not mandatory, but encouraged)