ref(browser): Improve fetchTransport error handling (#17661)

This changes our fetch transport a bit, making use of async functions
and slightly adjusting error handling:

1. Technically there was a bug if fetch is not available, as we would
keep increasing the `pendingBodySize`/`pendingCount` and never decrease
it.
2. We had some dedicated error messages for edge cases that are IMHO not
necessary - we can safe a few bytes by just using default error messages
there.
3. No need to use sync promise here, all of this is async anyhow.

Extracted this out of
#17641

Author: mydea

packages/browser/src/transports/fetch.ts

@@ -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';
@@ -9,12 +9,12 @@ import type { BrowserTransportOptions } from './types';
  */
 export function makeFetchTransport(
   options: BrowserTransportOptions,
-  nativeFetch: typeof WINDOW.fetch | undefined = getNativeImplementation('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++;
@@ -39,29 +39,23 @@ export function makeFetchTransport(
       ...options.fetchOptions,
     };
 
-    if (!nativeFetch) {
-      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);
     }
   }
 

packages/browser/test/transports/fetch.test.ts

@@ -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';
 
@@ -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({
@@ -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({