[workers-utils] Move shared binding helpers into workers-utils#12948
[workers-utils] Move shared binding helpers into workers-utils#12948
Conversation
🦋 Changeset detectedLatest commit: 019587c 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
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: |
Pure file moves with no content changes. These will be adapted in the next commit.
penalosa
force-pushed
the
penalosa/shared-binding-helpers
branch
2 times, most recently
from
1bfe7f8 to
048c516
Compare
- Update all wrangler import sites to import directly from @cloudflare/workers-utils (no re-export shims) - access.ts: add optional AccessLogger parameter, replace wrangler logger import - create-worker-upload-form.ts: fix self-referencing imports to use local paths - capnp.ts: fix self-referencing import - binding-helpers.ts: new file with extractBindingsOfType, isUnsafeBindingType, FlatBinding - Delete old wrangler files (access.ts, create-worker-upload-form.ts, capnp.ts)
penalosa
force-pushed
the
penalosa/shared-binding-helpers
branch
from
048c516 to
019587c
Compare
|
/bonk I don't think the PR description here is correct anymore. Can you update it? |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The refactoring is structurally sound — the file moves, import updates, and type adjustments are all correct. A few observations:
Issues
1. Changeset description is inaccurate
The changeset says:
Wrangler re-exports them at the original paths for backward compatibility.
This is not true. packages/wrangler/src/user/access.ts and packages/wrangler/src/deployment-bundle/create-worker-upload-form.ts were completely deleted — there are no re-export shims. This is fine since the package is private and all internal consumers were updated, but the changeset text is misleading.
2. extractBindingsOfType / isUnsafeBindingType are duplicated, not moved
The changeset claims these "now live in @cloudflare/workers-utils", but the original implementations in packages/wrangler/src/api/startDevWorker/utils.ts were left in place. Multiple wrangler files still import from the old location (./utils). Only the new create-worker-upload-form.ts in workers-utils uses the workers-utils copy.
This might be intentional (incremental migration for PR 2), but worth noting that there are now two independent copies of these functions.
3. undici is external but only in devDependencies
undici is listed in tsup.config.ts's external array (line 18), meaning it won't be bundled — it remains as a bare import "undici" in the built output. However, it's only in devDependencies. Since the package is "private": true and only consumed within the monorepo (where undici is hoisted by pnpm), this works in practice. But it's technically incorrect: an external dep that's needed at runtime should be in dependencies, or it should be removed from the external array so tsup bundles it.
command-exists is handled correctly — it's NOT in the external array, so it gets bundled, and being in devDependencies is consistent with the monorepo convention.
Looks good
- The
AccessLoggerinterface abstraction is clean — it decouplesaccess.tsfrom wrangler's logger singleton while keeping the API simple with a noop default. - The type change from
StartDevWorkerInput["bindings"]toRecord<string, Binding> | undefinedis type-identical (verified the source types), and correctly decouples the function from wrangler-specific types. - The
usesAccessCacheMap type annotation (Map<string, boolean>) and thematches[1] !== undefinedcheck are nice improvements over the original. - All internal import paths in wrangler were properly updated — no stale references to removed files.
|
Review posted on PR #12948. Here's a summary of the findings: Three issues identified:
The core refactoring is correct — the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Stale vi.mock path causes test mock to not intercept getAccessToken
The test imports getAccessToken from @cloudflare/workers-utils (line 2) and RemoteRuntimeController.ts also now imports it from @cloudflare/workers-utils (line 1 of that file). However, the vi.mock on line 38 still targets the old, deleted path "../../../user/access". This mock has no effect because no production code imports from that path anymore. As a result, vi.mocked(getAccessToken).mockResolvedValue(undefined) on line 167 will throw at runtime because getAccessToken is the real (non-mock) function from @cloudflare/workers-utils, not a vi.fn(). This will cause all tests in this file that depend on the mocked getAccessToken to fail.
(Refers to lines 38-41)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
/bonk can you fix this?
| @@ -1,4 +1,5 @@ | |||
| /* eslint-disable workers-sdk/no-vitest-import-expect -- expect used in vi.waitFor callbacks */ | |||
| import { getAccessToken } from "@cloudflare/workers-utils"; | |||
There was a problem hiding this comment.
/bonk is this mock working?
2 participants
Move shared helpers into
@cloudflare/workers-utilsso they can be reused across packages.getAccessToken/domainUsesAccess: moved fromwrangler/src/user/access.tswith an optionalloggerparameter. Wrangler'saccess.tsbecomes a thin wrapper that passes its logger.toWorkerMetadataBindings: new shared function that convertsRecord<string, Binding>toWorkerMetadataBinding[]for the Worker upload API. Wrangler'screateWorkerUploadFormnow delegates to it instead of inline conversion.This is PR 1 of 2 — PR 2 adds
@cloudflare/remote-bindingswhich reuses these shared helpers.