Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 15 additions & 21 deletions packages/browser/src/transports/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/core';
import { createTransport, rejectedSyncPromise } from '@sentry/core';
import { createTransport } from '@sentry/core';
import { clearCachedImplementation, getNativeImplementation } from '@sentry-internal/browser-utils';
import type { WINDOW } from '../helpers';
import type { BrowserTransportOptions } from './types';
Expand All @@ -9,12 +9,12 @@ import type { BrowserTransportOptions } from './types';
*/
export function makeFetchTransport(
options: BrowserTransportOptions,
nativeFetch: typeof WINDOW.fetch | undefined = getNativeImplementation('fetch'),
Copy link
Member Author

Choose a reason for hiding this comment

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

this type was not what we expected it to be, as nativeFetch was interpreted to be typeof fetch anyhow because the undefined just lead to it assuming the return type of getNativeImplementation('fetch'), which was also typeof fetch 😅

nativeFetch: typeof WINDOW.fetch = getNativeImplementation('fetch'),
): Transport {
let pendingBodySize = 0;
let pendingCount = 0;

function makeRequest(request: TransportRequest): PromiseLike<TransportMakeRequestResponse> {
async function makeRequest(request: TransportRequest): Promise<TransportMakeRequestResponse> {
const requestSize = request.body.length;
pendingBodySize += requestSize;
pendingCount++;
Expand All @@ -39,29 +39,23 @@ export function makeFetchTransport(
...options.fetchOptions,
};

if (!nativeFetch) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this lead to pendingXX not being cleaned up in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

clearCachedImplementation('fetch');
return rejectedSyncPromise('No fetch implementation available');
}

try {
// Note: We do not need to suppress tracing here, becasue we are using the native fetch, instead of our wrapped one.
return nativeFetch(options.url, requestOptions).then(response => {
pendingBodySize -= requestSize;
pendingCount--;
return {
statusCode: response.status,
headers: {
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
'retry-after': response.headers.get('Retry-After'),
},
};
});
// Note: We do not need to suppress tracing here, because we are using the native fetch, instead of our wrapped one.
const response = await nativeFetch(options.url, requestOptions);

return {
statusCode: response.status,
headers: {
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
'retry-after': response.headers.get('Retry-After'),
},
};
} catch (e) {
clearCachedImplementation('fetch');
throw e;
} finally {
pendingBodySize -= requestSize;
pendingCount--;
return rejectedSyncPromise(e);
}
}

Expand Down
25 changes: 21 additions & 4 deletions packages/browser/test/transports/fetch.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { EventEnvelope, EventItem } from '@sentry/core';
import { createEnvelope, serializeEnvelope } from '@sentry/core';
import type { Mock } from 'vitest';
import { describe, expect, it, vi } from 'vitest';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { makeFetchTransport } from '../../src/transports/fetch';
import type { BrowserTransportOptions } from '../../src/transports/types';

Expand Down Expand Up @@ -29,7 +29,11 @@ class Headers {
}
}

describe('NewFetchTransport', () => {
describe('fetchTransport', () => {
afterEach(() => {
vi.resetAllMocks();
});

it('calls fetch with the given URL', async () => {
const mockFetch = vi.fn(() =>
Promise.resolve({
Expand Down Expand Up @@ -102,15 +106,28 @@ describe('NewFetchTransport', () => {
});
});

it('handles when `getNativetypeof window.fetchementation` is undefined', async () => {
it('handles when native fetch implementation returns undefined', async () => {
const mockFetch = vi.fn(() => undefined) as unknown as typeof window.fetch;
const transport = makeFetchTransport(DEFAULT_FETCH_TRANSPORT_OPTIONS, mockFetch);

expect(mockFetch).toHaveBeenCalledTimes(0);
await expect(() => transport.send(ERROR_ENVELOPE)).not.toThrow();
await expect(() => transport.send(ERROR_ENVELOPE)).rejects.toThrow(
"Cannot read properties of undefined (reading 'status')",
);
expect(mockFetch).toHaveBeenCalledTimes(1);
});

it('handles when native fetch implementation is undefined', async () => {
vi.mock('@sentry-internal/browser-utils', async importOriginal => ({
...(await importOriginal()),
getNativeImplementation: () => undefined,
}));

const transport = makeFetchTransport(DEFAULT_FETCH_TRANSPORT_OPTIONS);

await expect(() => transport.send(ERROR_ENVELOPE)).rejects.toThrow('nativeFetch is not a function');
});

it('correctly sets keepalive flag', async () => {
const mockFetch = vi.fn(() =>
Promise.resolve({
Expand Down