Skip to content

Conversation

@penalosa
Copy link
Contributor

Fixes DEVX-2260

Modify the input types of constructWranglerConfig() from @cloudlfare/workers-utils


  • Tests
    • Tests included
    • Tests not necessary because: Covered by the existing wrangler init --from-dash tests
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal refactor
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: we're not backporting workers-utils

@penalosa penalosa requested a review from a team as a code owner October 31, 2025 13:03
@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2025

🦋 Changeset detected

Latest commit: 157f921

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

@penalosa penalosa added the skip-v3-pr Skip validation of presence of a v3 backport PR label Oct 31, 2025
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Oct 31, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 31, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@11146

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@11146

miniflare

npm i https://pkg.pr.new/miniflare@11146

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@11146

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@11146

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@11146

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@11146

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@11146

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@11146

wrangler

npm i https://pkg.pr.new/wrangler@11146

commit: 157f921

Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🙂

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Nov 3, 2025
@penalosa penalosa requested a review from 1000hz November 3, 2025 11:29
@petebacondarwin petebacondarwin marked this pull request as draft November 11, 2025 15:48
@penalosa penalosa force-pushed the penalosa/match-api-types branch from ad825ea to 4921dfa Compare November 11, 2025 16:39
@penalosa penalosa marked this pull request as ready for review November 11, 2025 16:40
@penalosa
Copy link
Contributor Author

@claude Can you review and suggest improvements to this PR, focusing only on the changeset?

@claude
Copy link
Contributor

claude bot commented Nov 12, 2025

Claude finished @penalosa's task —— View job


PR Review Progress

Reviewing the changeset for constructWranglerConfig() type changes:

  • Read the changeset to understand scope
  • Review main changes in construct-wrangler-config.ts
  • Examine type modifications and their implications
  • Check integration with wrangler and tests
  • Provide focused feedback on changeset

Changeset Review: constructWranglerConfig() Type Changes

Summary: This PR refactors constructWranglerConfig() to accept a more structured APIWorkerConfig interface instead of individual parameters, better aligning with dash API types.

Key Changes

  1. Interface Transformation:

    • Replaced FullWorkerConfig with APIWorkerConfig that matches actual API response structure
    • Consolidated scattered API response types into a single, well-documented interface
  2. Function Signature Simplification:

    // Before
    constructWranglerConfig(workerName: string, entrypoint: string, config: FullWorkerConfig)
    
    // After  
    constructWranglerConfig(workerOrWorkers: APIWorkerConfig | APIWorkerConfig[])
  3. Data Mapping Improvements:

    • Direct property access (config.name, config.entrypoint) instead of parameter passing
    • Simplified domain mapping using config.domains vs separate customDomains
    • More direct API-to-config property mappings

Review Feedback

Strengths

  • Better API alignment: The new APIWorkerConfig interface closely matches actual Cloudflare API responses
  • Improved encapsulation: All worker data is contained within a single object
  • Cleaner integration: The wrangler integration code now explicitly maps API responses to the expected format
  • Good documentation: API endpoints are clearly documented in interface comments

⚠️ Issues to Address

  1. Formatting Issue (Line 35): There's a misplaced line break that should be fixed:

    // packages/workers-utils/src/construct-wrangler-config.ts:35
    domains: Cloudflare.Workers.Domain[] /* comment should be on new line */;
  2. DO Binding Filtering (Lines 54-58): The current code doesn't filter DO bindings by script_name:

    const durableObjectClassNames = config.bindings
      .filter((binding) => binding.type === "durable_object_namespace")
      // Missing: && binding.script_name === config.name

    This could include migrations for classes not exported by the current script.

  3. Service Tag Logic (Line 147): Incorrect comparison:

    // Current (incorrect)
    if (serviceTag !== workerName || envTag === undefined) {
    
    // Should be
    if (serviceTag !== `${SERVICE_TAG_PREFIX}${workerName}` || envTag === undefined) {

📝 Minor Suggestions

  • Consider making placement property more type-safe rather than using optional chaining
  • The logpush: undefined assignment in wrangler integration seems unnecessary

Changeset Assessment

Appropriate Version Bump: ✅ Minor is correct - this changes the public API of @cloudflare/workers-utils in a backwards-incompatible way, but it's an internal utility primarily used by wrangler.

Testing Coverage: ✅ The existing wrangler init --from-dash tests should adequately cover this change.

Overall Assessment: This is a solid refactor that improves type safety and API alignment. The identified issues should be addressed, but the core approach is sound.

@penalosa penalosa merged commit d7a2037 into main Nov 12, 2025
63 of 75 checks passed
@penalosa penalosa deleted the penalosa/match-api-types branch November 12, 2025 16:17
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-v3-pr Skip validation of presence of a v3 backport PR

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants