Repository navigation
fix(gaxios): remove --bun-plugin-shim and support Bun fetch natively - #9561
danieljbruce wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the Bun plugin shim and introduces a native Bun fetch wrapper in gaxios to handle stream conversions between Node.js and Web streams. The review feedback highlights two important issues: first, error events should be explicitly forwarded when piping streams to a PassThrough to avoid silent failures; second, res.arrayBuffer() should be overridden alongside res.text() and res.json() to prevent stream locking errors when standard response methods are invoked.
| const stream = | ||
| fetchInit.body instanceof Readable | ||
| ? fetchInit.body | ||
| : (fetchInit.body as Readable).pipe(new PassThrough()); |
There was a problem hiding this comment.
When piping a non-Readable stream (which has a .pipe method) to a PassThrough stream, any errors emitted by the source stream will not be automatically propagated to the PassThrough stream. This can lead to silent failures or unhandled stream errors during the fetch request. We should explicitly forward the 'error' event from the source stream to the destination stream.
const source = fetchInit.body as Readable;
let stream: Readable;
if (source instanceof Readable) {
stream = source;
} else {
stream = source.pipe(new PassThrough());
if (typeof source.on === 'function') {
source.on('error', err => stream.emit('error', err));
}
}| const origText = res.text.bind(res); | ||
| const origJson = res.json.bind(res); | ||
| Object.defineProperty(res, 'body', { | ||
| get() { | ||
| nodeStream ||= Readable.fromWeb(rawBody); | ||
| return nodeStream; | ||
| }, | ||
| configurable: true, | ||
| enumerable: true, | ||
| }); | ||
| res.text = async () => { | ||
| if (!nodeStream) return origText(); | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of nodeStream) { | ||
| chunks.push( | ||
| Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array), | ||
| ); | ||
| } | ||
| return Buffer.concat(chunks).toString('utf8'); | ||
| }; | ||
| res.json = async () => { | ||
| if (!nodeStream) return origJson(); | ||
| return JSON.parse(await res.text()); | ||
| }; |
There was a problem hiding this comment.
When res.body is accessed, Readable.fromWeb(rawBody) is called, which locks the underlying Web ReadableStream. If a caller subsequently invokes res.arrayBuffer(), the native arrayBuffer() method will fail with a TypeError: body stream is locked error. To ensure full compatibility with standard Response methods (as noted in the repository's general rules), we should also override res.arrayBuffer to read from nodeStream when it is defined.
const origText = res.text.bind(res);
const origJson = res.json.bind(res);
const origArrayBuffer = res.arrayBuffer.bind(res);
Object.defineProperty(res, 'body', {
get() {
nodeStream ||= Readable.fromWeb(rawBody);
return nodeStream;
},
configurable: true,
enumerable: true,
});
res.text = async () => {
if (!nodeStream) return origText();
const chunks: Buffer[] = [];
for await (const chunk of nodeStream) {
chunks.push(
Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array),
);
}
return Buffer.concat(chunks).toString('utf8');
};
res.json = async () => {
if (!nodeStream) return origJson();
return JSON.parse(await res.text());
};
res.arrayBuffer = async () => {
if (!nodeStream) return origArrayBuffer();
const chunks: Buffer[] = [];
for await (const chunk of nodeStream) {
chunks.push(
Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array),
);
}
const buf = Buffer.concat(chunks);
return buf.buffer.slice(
buf.byteOffset,
buf.byteOffset + buf.byteLength,
);
};References
- When using google-auth-library v11+ (which uses gaxios v7+), GaxiosResponse supports standard Response methods like arrayBuffer().
…r/blob, and proxy/tls in #createBunFetch
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the Bun plugin shim and introduces native support for Bun's fetch environment in Gaxios. It adds a custom Bun fetch wrapper (#createBunFetch) to handle the conversion of Node.js Readable request bodies to Web ReadableStreams, and Web ReadableStream response bodies back to Node.js Readable streams. Additionally, it ensures proxy and TLS options are correctly passed to the fetch implementation. The review feedback highlights an issue in #createBunFetch where returning early when __googleCloudBunFetch is defined bypasses the stream conversion logic. The reviewer suggests resolving a fetchImpl reference to ensure stream conversions are consistently applied and tested.
| const bunFetch = (globalThis as {__googleCloudBunFetch?: typeof fetch}) | ||
| .__googleCloudBunFetch; | ||
| if (typeof bunFetch === 'function') { | ||
| return bunFetch(input, init); | ||
| } |
There was a problem hiding this comment.
The current implementation returns early when __googleCloudBunFetch is defined, completely bypassing the request and response stream conversion logic. According to the PR description, the intention is to delegate to globalThis.fetch (or globalThis.__googleCloudBunFetch when --fetch-shim is active) while still performing the stream conversions. By resolving a fetchImpl reference instead of returning early, we can ensure that the stream conversion logic is consistently applied and fully tested under the test suite's --fetch-shim environment.
const bunFetch = (globalThis as {__googleCloudBunFetch?: typeof fetch})
.__googleCloudBunFetch;
const fetchImpl = typeof bunFetch === 'function' ? bunFetch : globalThis.fetch;| const res = await globalThis.fetch( | ||
| input, | ||
| fetchInit as RequestInit | undefined, | ||
| ); |
There was a problem hiding this comment.
Use the resolved fetchImpl instead of hardcoding globalThis.fetch to ensure that the shimmed fetch is called when active, while still benefiting from the stream conversion logic.
const res = await fetchImpl(
input,
fetchInit as RequestInit | undefined,
);This branch has not been deployed
1 participant
Description
Removes the
--bun-plugin-shim(Bun.pluginonLoadsource rewriting) monkey patch frombin/proxyquire-bun-shim.cjsandbin/run-test.cjs, and updatesgaxiosto route default fetch calls on Bun throughGaxios.#createBunFetch()instead ofimport('node-fetch')(b/570680331).Previously,
Gaxios.#getFetch()evaluated(await import('node-fetch')).defaultin non-browser runtimes. In Bun,import('node-fetch')is hijacked by Bun's internalbun:node-fetchpolyfill, which calls Bun's internal C++ fetch directly and bypassesglobalThis.fetch(as well asnockv14'sglobalThis.fetchinterception) and fails to convert Nodestream.Readablerequest bodies or return Nodestream.Readableresponse bodies forresponseType: 'stream'. To work around this,bin/proxyquire-bun-shim.cjsregistered aBun.pluginonLoadhook that rewrotebuild/esm/src/gaxios.json disk at load time.Impact
core/packages/gaxios:Gaxios.#getFetch()now uses#createBunFetch()when running under Bun ('Bun' in globalThis && typeof globalThis.fetch === 'function'), delegating toglobalThis.fetch(orglobalThis.__googleCloudBunFetchwhen--fetch-shimis active), converting NodeReadablerequest bodies viaReadable.toWeb, and lazily wrapping WebReadableStreamresponse bodies viaReadable.fromWeb.bin/proxyquire-bun-shim.cjs&bin/run-test.cjs: Removes--bun-plugin-shim(BUN_PLUGIN_SHIM/BUN_ENABLE_BUN_PLUGIN_SHIM) and theBun.pluginonLoadsource-rewriting hook.Changes
core/packages/gaxios/src/gaxios.ts: AddGaxios.#createBunFetch()and use it inGaxios.#getFetch()when running on Bun; forwardproxyandtlsoptions tofetchImplementationin_defaultAdapterand handle non-ErrorAbortController.abort(reason)values.core/packages/gaxios/test/test.getch.ts: Add unit test coverage forproxy/tlsforwarding tofetchImplementationand non-Errorabort reasons.core/packages/gaxios/package.json,bin/run-test.cjs,bin/proxyquire-bun-shim.cjs: Remove--bun-plugin-shimand its implementation.Testing
pnpm --dir core/packages/gaxios testunder both Node.js and Bun (JS_RUNTIME=bun).Alternatives
Bun.pluginonLoadstring-replacement hook inbin/proxyquire-bun-shim.cjswas rejected because rewriting compiled JS files at module load time is fragile and only worked inside the test runner rather than fixinggaxioson Bun directly (b/570091189).