-
-
Notifications
You must be signed in to change notification settings - Fork 414
fix: ESM compatibility - avoid direct Response object mutation #1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
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.
ef1631a to
839fb08
Compare
|
bugbot run |
|
@codex review |
|
@omarmciver Could you add a changeset? |
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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
_dataand_errorproperties 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; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
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.
| 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; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
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.
| 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.
Summary
Fixes #1429 - Response object mutation fails in ESM environments
Problem
The Fetch client template directly mutates Response objects by adding
dataanderrorproperties. 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:
dataanderrorpropertiesChanges
Modified
templates/base/http-clients/fetch-http-client.ejsto create a wrapper object instead of directly mutating the Response:Testing
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:
"type": "module"in package.json)Without this fix, users need workarounds like wrapping fetch with a Proxy to make Response properties writable, which is not ideal.