Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 5a0fe56 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
Hi @AyobamiH, thank you for the PR 🙏 As you can see CI is very red, could you address it? |
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
373a7e8 to
09d713e
Compare
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Thanks for the PR @AyobamiH 🙏
I do believe that this would be introducing a bunch of duplicated code (as per my comment), could you have a look and see if this can be avoided? 🙏
| import { createCLIParser } from "./index"; | ||
| import type { DefinitionTreeNode } from "./core/types"; | ||
|
|
||
| export type ExperimentalCommandMetadata = { |
There was a problem hiding this comment.
this type is just duplicating this already-existing type:
I think this will make this code pretty problematic to maintain and keep in sync, I think we can just move the metadata type (and any other type you're re-defining in this file) in its own shared module and import it both into this file and into the original file.
ensuring that we're not duplicating code while still resolving the issue
would that be possible?
There was a problem hiding this comment.
Thanks for pointing this out, Dario — agreed. Duplicating the internal registry types in the experimental API would make future changes prone to drift.
I've refactored this at 1ebe991 following your suggestion. Both core/types.ts and experimental-commands-api.ts now import the same definitions from command-types.ts for metadata, command identity, shared behaviour, command data, namespace/alias definitions and the recursive tree. The internal Teams/category constraints stay intact, and the published API remains self-contained.
The one intentional structural projection is the public argument-options type: the internal ArgDefinition is Yargs-derived, and exposing Yargs in Wrangler's published declarations would reintroduce the original private-dependency leak. I've documented that distinction and added a type-level regression test.
I validated the new head locally with a full dependent-package build, Wrangler source and E2E type checks, 16 targeted API/declaration tests, command-registration and completion tests, type-aware lint, formatting and the published-declaration audit for Wrangler and workers-utils. The upstream checks are running independently for this commit.
Could you take another look at the shared approach? Thanks again for the direction.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
This pull request was not reviewed automatically because its author does not have write access to this repository. A maintainer with write access can start a review by commenting |
|
@dario-piotrowicz — following up on your 9 October review: the shared-type refactor is in place ( I've also reconciled the PR with the latest upstream On this exact head, the frozen pnpm lockfile check and workspace dependency installation (scripts disabled) passed, as did the targeted tests: 16/16 Wrangler and 23/23 workers-utils. Upstream CI is running independently and has not yet fully completed. Could you give the shared-type approach another look after the checks settle? I'll leave approval and merge to you and the Wrangler code owners. Thanks! |
Addresses the Wrangler declaration leaks in #15940. The separate Miniflare declaration errors remain tracked in #15377.
Problem
Wrangler's published declarations reference private workspace packages and development-only dependencies. Following only their top-level imports can also leave unresolved relative paths in the generated
cli.d.ts. Consumers that check dependency declarations withskipLibCheck: falsethen fail to compile.Inlining every package is insufficient: workers-utils owns the unique symbol used by inherited bindings, and Undici exposes its
MockAgentclass through Wrangler's public worker options. These types must retain their dependency-owned identities.Changes
noExternalsettings preserve Wrangler's bundled CommonJS runtime.experimental_getWranglerCommandsan explicit, dependency-free structural return type for its command registry and global flags, keeping internal command implementation types out of the published declarations.Validation
Maintainer review follow-up (12 October,
1ebe991/33b96e34)The 9 October review was correct: duplicating registry type definitions would be a maintenance burden. Internal
core/types.tsand the experimental command API now both import the same shapes fromsrc/command-types.ts, covering metadata, identity, command data, behaviour, aliases, namespaces and recursive registry nodes. Internal Teams/category unions remain constrained.The public
ExperimentalCommandArgDefinitionremains a deliberate Yargs-free structural projection, not a second copy of internalArgDefinition: the internal one is derived from development-only Yargs declarations and exporting it would recreate the package-boundary leak. No command execution or runtime behaviour was changed.New-head local validation (
1ebe991): full Wrangler dependency build; source and E2E TypeScript compilations; 16 targeted experimental API/published-declaration tests; adjacent command-registration and completion tests; format, type-aware lint, and diff checks. The published declaration audit covered Wrangler (3 type entry points) and workers-utils (11 type entry points) with no unresolved imports. See GitHub Checks for new-head upstream CI.Latest merged-main validation (
33b96e34, 12 October): merged upstreammainat9be5ec7to remove the existing merge conflict incore/types.ts. Preserved upstream's updated temporary-account/event-code documentation in the shared type module without restoring duplicated fields. Fresh frozen workspace install; full Wrangler dependent-package build (Wrangler 4.149.0); source and E2E TypeScript compilation with--noEmit; generated declaration audit across Wrangler (3 published type entry points) and workers-utils (11); four targeted suites (48 passed, 2 skipped, 0 failed); changed-file type-aware lint and diff validation all passed. The fork branch and worktree were clean after the merge commit. Upstream GitHub Actions has been triggered for this latest head and must independently pass before merge.Bot review cross-check (12 October,
02f9a5ec)Older Devin observations have also been rechecked against the current dependency strategy rather than relying on historical "resolved" labels:
unstable_devand an external installed UndiciMockAgent/setGlobalDispatcher. Wrangler'sfetch()returned the exact intercepted response andassertNoPendingInterceptors()passed. This is runtime evidence in addition to the declaration-levelMockAgenttype assertion.cloudflare/workers-utilsproduction dependency: an earlier automated note was marked resolved when workers-utils was temporarily a devDependency, but that condition is no longer true. The current runtime intentionally bundles workers-utils while its published types import the installed package to preserveINHERIT_SYMBOL's nominal identity. Removing the production dependency without restructuring the public declaration contract would reintroduce consumer type resolution errors. The present dependency is therefore deliberate and should be reviewed as a compatibility trade-off, not described as removed.description,nargs,group,conflictsandimplieswithout requiring Yargs at install time.supportTemporaryJSDoc default was restored in02f9a5ec; TypeScript AST inspection confirms the recogniseddefault falsetag and Devin marked the finding resolved. All four targeted suites passed again (48 passed, 2 skipped).The Cloudflare Wrangler code-owner approval and full upstream CI remain separate merge gates.
Pre-review validation (
ee458de)The pre-review candidate at
ee458dematched the tested fork commita2f952e6ef5b0d3b53b966602bbefb72b585a652; those results are historical rather than evidence for the latest head. Fork-only workflow files are excluded.type:tests, workers-utils type tests, changed-file type-aware lint, and formatting passed locally.ee458depackages with TypeScript 5.8.3,strict: true, andskipLibCheck: falseremoved all nine PR-specific declaration errors: utilities went from five errors to zero; Wrangler went from nineteen to ten. The remaining ten diagnostics exactly match the unchanged Miniflare-only control.Binding,INHERIT_SYMBOL, andMockAgentcompatibility, reject unrelated unique symbols, and check against accidentalany.4.148.0successfully. Its installed files and the utilities files match their immutable prerelease archives byte for byte.unstable_dev; Wrangler'sfetch()then used an external UndiciMockAgentas its global dispatcher, returned the exact mocked response, and consumed every registered interceptor.ee458de.Checklist