Skip to content

Conversation

@omarmciver
Copy link

Summary

Fixes #1429 - Response object mutation fails in ESM environments

Problem

The Fetch client template directly mutates Response objects by adding data and error properties. This works in CommonJS but fails in ESM with node-fetch v3 where Response objects are truly read-only.

Solution

Instead of mutating the Response object, this PR creates a wrapper object that:

  • Contains the custom data and error properties
  • Delegates all Response properties and methods to the original response
  • Maintains full backward compatibility with existing code
  • Works in both CommonJS and ESM environments

Changes

Modified templates/base/http-clients/fetch-http-client.ejs to create a wrapper object instead of directly mutating the Response:

// Before (fails in ESM):
const r = response as HttpResponse<T, E>;
r.data = null;
r.error = null;

// After (works everywhere):
const r = {
  data: null,
  error: null,
  // Delegate Response properties
  ok: response.ok,
  status: response.status,
  // ... etc
  // Delegate Response methods
  json: () => response.json(),
  text: () => response.text(),
  // ... etc
} as HttpResponse<T, E>;

Testing

  • ✅ Created test script to verify the template generates correct code
  • ✅ Tested that the fix doesn't directly mutate Response objects
  • ✅ Verified wrapper object is created properly
  • ✅ Confirmed backward compatibility is maintained

Breaking Changes

None - this change is fully backward compatible. The generated API clients will have the same interface and behavior, but will now work in ESM environments.

Additional Notes

This fix enables users to migrate to:

  • ESM modules ("type": "module" in package.json)
  • node-fetch v3 (ESM-only version)
  • Native fetch in Node.js 18+

Without this fix, users need workarounds like wrapping fetch with a Proxy to make Response properties writable, which is not ideal.

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2025

⚠️ No Changeset found

Latest commit: 837a381

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

When using fetch in ESM environments (e.g., node-fetch v3+), the Response
object is read-only and attempting to set properties like 'data' and 'error'
directly throws an error.

This fix creates a wrapper object that delegates to the original Response
while allowing the assignment of custom properties, ensuring compatibility
with both CommonJS and ESM environments.

Fixes compatibility with node-fetch v3+ and other ESM-only fetch implementations.
@omarmciver omarmciver force-pushed the fix/esm-response-mutation branch from ef1631a to 839fb08 Compare September 23, 2025 14:03
@smorimoto
Copy link
Collaborator

bugbot run

@smorimoto
Copy link
Collaborator

@codex review

@smorimoto
Copy link
Collaborator

@omarmciver Could you add a changeset?

@smorimoto smorimoto added the bug Something isn't working label Oct 4, 2025
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This PR is being reviewed by Cursor Bugbot

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Replace static property wrapper with Proxy-based delegation to fix
critical issues identified in automated reviews:

- Fix stale bodyUsed property (P1 bug)
- Ensure all Response properties remain live/dynamic
- Automatically include all Response methods (including stream())
- Maintain correct 'this' binding for delegated methods
- Preserve prototype chain and type checking

The Proxy approach provides true delegation while preventing Response
mutation, ensuring compatibility with both CommonJS and ESM environments.

Addresses feedback on PR acacode#1430
All snapshot tests updated to reflect the new Proxy implementation.
The generated code now uses Proxy for dynamic property delegation
instead of static property copying.

All 133 tests passing with updated snapshots.
@smorimoto smorimoto requested a review from Copilot October 23, 2025 07:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes ESM compatibility issues by replacing direct mutation of Response objects with a Proxy-based wrapper approach. The change addresses failures in ESM environments where Response objects are read-only, while maintaining full backward compatibility.

  • Replaces direct Response object mutation with a Proxy wrapper
  • Uses internal _data and _error properties to store custom values
  • Delegates all Response properties and methods to the original object

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.

File Description
templates/base/http-clients/fetch-http-client.ejs Modified HTTP client template to use Proxy instead of direct mutation
tests/**/snapshots/*.snap Updated test snapshots to reflect the new Proxy-based implementation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

get(target, prop) {
// Custom properties for our API wrapper
if (prop === 'data') {
return target._data !== undefined ? target._data : (null as unknown) as T;
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The ternary operator with type assertions is hard to read. Consider simplifying to return target._data ?? (null as unknown as T); for better clarity.

Suggested change
return target._data !== undefined ? target._data : (null as unknown) as T;
return target._data ?? (null as unknown as T);

Copilot uses AI. Check for mistakes.
}

// Prevent mutation of Response properties (ESM safety)
return false;
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The setter silently fails for non-data/error properties. Consider adding a comment explaining that this prevents accidental mutation of Response properties, or throwing an error to make the failure explicit.

Suggested change
return false;
throw new Error(
`Cannot set property '${String(prop)}' on HttpResponse: only 'data' and 'error' can be set.`
);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: Response object mutation fails in ESM environments with node-fetch v3

2 participants