add deprecation script#14631
Conversation
|
|
Review posted successfully to PR #14631 with a summary and 3 ranked issues (2 as inline suggestion comments):
|
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
2d3f40c to
fa50094
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
| for (const name of packageNames) { | ||
| if (!(name in graph)) { | ||
| throw new Error( | ||
| `"${name}" is not a known package. Known packages:\n ${knownPackages.join(", ")}` | ||
| ); | ||
| } | ||
| for (const dep of graph[name]) { | ||
| if (!provided.has(dep) && !missing.includes(dep)) { | ||
| missing.push(dep); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Dependency check reports incomplete list of required packages, forcing users to re-run multiple times
The missing-dependants check (getRequiredDependants at tools/deployments/deprecate.ts:99-110) only iterates over the user-provided package names, not the discovered missing ones, so transitive dependants are never reported in a single pass.
Impact: Users deprecating a low-level package may need to re-run the tool multiple times before all required co-deprecations are surfaced.
Transitive closure is not computed — only direct dependants of provided packages are checked
The function iterates for (const name of packageNames) and collects dependants of each, but never recurses into the collected missing list. In the real repo graph:
@cloudflare/kv-asset-handlerhas dependantwranglerwranglerhas dependants@cloudflare/vite-plugin,@cloudflare/vitest-pool-workers
When a user runs deprecate-packages @cloudflare/kv-asset-handler@latest, the function only reports wrangler as missing. After the user adds wrangler and re-runs, it then reports @cloudflare/vite-plugin and @cloudflare/vitest-pool-workers. The error message at tools/deployments/deprecate.ts:408 says "Re-run with versions for all affected packages" but doesn't provide the complete list.
Similarly for @cloudflare/unenv-preset → wrangler → @cloudflare/vitest-pool-workers.
A fix would be to iterate until a fixed point (or use a queue/BFS), adding newly-discovered missing dependants to the iteration set.
Prompt for agents
The getRequiredDependants function in tools/deployments/deprecate.ts:91-113 only checks direct dependants of the user-provided packageNames, but does not compute the transitive closure. This means if A depends on B depends on C, and the user only provides A, the function reports B as missing but not C.
To fix this, change the iteration to use a queue/worklist approach: start with the user-provided packageNames, and for each missing dependant discovered, also check ITS dependants. Continue until no new missing dependants are found. A simple approach is to use a while loop with a queue:
1. Initialize a queue with all packageNames
2. Pop from queue, check dependants in graph[name]
3. If a dependant is not in provided set and not already in missing set, add to missing and also add to queue
4. Continue until queue is empty
This ensures all transitive dependants are reported in a single invocation. The existing tests would still pass since the TEST_GRAPH happens to have all transitive dependants also as direct dependants of miniflare, but a new test case should be added for a true transitive chain (e.g., A -> B -> C where C is not a direct dependant of A).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
i don't think this is worth the effort - its highly unlikely we're going to deprecate these packages
mini cli to deprecate multiple packages in lock step.
Examples:
Dependency rules (enforced automatically):
miniflare -> must also deprecate wrangler, vite-plugin, vitest-pool-workers
wrangler -> must also deprecate vite-plugin, vitest-pool-workers
npm dist-tag add <package>@<good-version> lateston the good version,npm deprecate <package>@<bad-version> <reason>you can also test this manually with --dry-run
A picture of a cute animal (not mandatory, but encouraged)