fix: ESM compatibility - avoid direct Response object mutation #1430
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.
omarmciver
force-pushed
the
fix/esm-response-mutation
branch
from
ef1631a to
839fb08
Compare
|
bugbot run |
|
@codex review |
|
@omarmciver Could you add a changeset? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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
| // Create a wrapper object that doesn't mutate the Response | ||
| // This ensures compatibility with ESM environments where Response is read-only | ||
| const r = { | ||
| data: (null as unknown) as T, | ||
| error: (null as unknown) as E, | ||
| // Delegate Response properties | ||
| ok: response.ok, | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers: response.headers, | ||
| url: response.url, | ||
| redirected: response.redirected, | ||
| type: response.type, | ||
| body: response.body, | ||
| bodyUsed: response.bodyUsed, | ||
| // Delegate Response methods |
There was a problem hiding this comment.
The new wrapper records response.bodyUsed once and stores the boolean on the returned object, but the response body is consumed later in the function when responseFormat is defined. After a call resolves, data.bodyUsed still reports the initial value (false) even though the underlying Response has been read and json()/text() can no longer be called without throwing. Previously, because the original Response instance was returned, bodyUsed reflected the consumed state. Any client code that checks bodyUsed to determine whether the body can be re-read will now be misled and may attempt a second read that fails.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| formData: () => response.formData(), | ||
| json: () => response.json(), | ||
| text: () => response.text(), | ||
| } as HttpResponse<T, E>; |
There was a problem hiding this comment.
Bug: HttpResponse Wrapper Causes Compatibility Problems
The new HttpResponse wrapper object introduces several compatibility issues. It's a plain object, which breaks instanceof Response checks. Properties like body and bodyUsed are statically copied, leading to stale values, and the stream() method is missing from the delegated Response methods.
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.