api-fetch: clean up error handling (#71458)

* api-fetch: add unit tests for nonce refresh, reset apiFetch module on each test

* api-fetch: clean up error handling

Author: jsnajdr

packages/api-fetch/src/index.ts

@@ -61,21 +61,6 @@ function registerMiddleware( middleware: APIFetchMiddleware ) {
 	middlewares.unshift( middleware );
 }
 
-/**
- * Checks the status of a response, throwing the Response as an error if
- * it is outside the 200 range.
- *
- * @param response
- * @return The response if the status is in the 200 range.
- */
-const checkStatus = ( response: Response ) => {
-	if ( response.status >= 200 && response.status < 300 ) {
-		return response;
-	}
-
-	throw response;
-};
-
 const defaultFetchHandler: FetchHandler = ( nextOptions ) => {
 	const { url, path, data, parse = true, ...remainingOptions } = nextOptions;
 	let { body, headers } = nextOptions;
@@ -89,7 +74,7 @@ const defaultFetchHandler: FetchHandler = ( nextOptions ) => {
 		headers[ 'Content-Type' ] = 'application/json';
 	}
 
-	const responsePromise = window.fetch(
+	const responsePromise = globalThis.fetch(
 		// Fall back to explicitly passing `window.location` which is the behavior if `undefined` is passed.
 		url || path || window.location.href,
 		{
@@ -101,13 +86,15 @@ const defaultFetchHandler: FetchHandler = ( nextOptions ) => {
 	);
 
 	return responsePromise.then(
-		( value ) =>
-			Promise.resolve( value )
-				.then( checkStatus )
-				.catch( ( response ) => parseAndThrowError( response, parse ) )
-				.then( ( response ) =>
-					parseResponseAndNormalizeError( response, parse )
-				),
+		( response ) => {
+			// If the response is not 2xx, still parse the response body as JSON
+			// but throw the JSON as error.
+			if ( ! response.ok ) {
+				return parseAndThrowError( response, parse );
+			}
+
+			return parseResponseAndNormalizeError( response, parse );
+		},
 		( err ) => {
 			// Re-throw AbortError for the users to handle it themselves.
 			if ( err && err.name === 'AbortError' ) {
@@ -116,7 +103,7 @@ const defaultFetchHandler: FetchHandler = ( nextOptions ) => {
 
 			// If the browser reports being offline, we'll just assume that
 			// this is why the request failed.
-			if ( ! window.navigator.onLine ) {
+			if ( ! globalThis.navigator.onLine ) {
 				throw {
 					code: 'offline_error',
 					message: __(
@@ -189,10 +176,17 @@ const apiFetch: apiFetch = ( options ) => {
 		}
 
 		// If the nonce is invalid, refresh it and try again.
-		return window
+		return globalThis
 			.fetch( apiFetch.nonceEndpoint! )
-			.then( checkStatus )
-			.then( ( data ) => data.text() )
+			.then( ( response ) => {
+				// If the nonce refresh fails, it means we failed to recover from the original
+				// `rest_cookie_invalid_nonce` error and that it's time to finally re-throw it.
+				if ( ! response.ok ) {
+					return Promise.reject( error );
+				}
+
+				return response.text();
+			} )
 			.then( ( text ) => {
 				apiFetch.nonceMiddleware!.nonce = text;
 				return apiFetch( options );

packages/api-fetch/src/middlewares/media-upload.ts

@@ -63,9 +63,9 @@ const mediaUploadMiddleware: APIFetchMiddleware = ( options, next ) => {
 	};
 
 	return next( { ...options, parse: false } )
-		.catch( ( response ) => {
+		.catch( ( response: Response ) => {
 			// `response` could actually be an error thrown by `defaultFetchHandler`.
-			if ( ! response.headers ) {
+			if ( ! ( response instanceof globalThis.Response ) ) {
 				return Promise.reject( response );
 			}
 
@@ -92,7 +92,7 @@ const mediaUploadMiddleware: APIFetchMiddleware = ( options, next ) => {
 			}
 			return parseAndThrowError( response, options.parse );
 		} )
-		.then( ( response ) =>
+		.then( ( response: Response ) =>
 			parseResponseAndNormalizeError( response, options.parse )
 		);
 };

packages/api-fetch/src/test/index.js

@@ -1,8 +1,3 @@
-/**
- * Internal dependencies
- */
-import apiFetch from '../';
-
 /**
  * Mock response value for a successful fetch.
  *
@@ -15,9 +10,15 @@ const DEFAULT_FETCH_MOCK_RETURN = {
 };
 
 describe( 'apiFetch', () => {
+	let apiFetch;
 	const originalFetch = globalThis.fetch;
 
-	beforeEach( () => {
+	beforeEach( async () => {
+		// Reset the `apiFetch` module before each test to clear
+		// internal variables (middlewares, fetch handler, etc.).
+		jest.resetModules();
+		apiFetch = ( await import( '../' ) ).default;
+
 		globalThis.fetch = jest.fn();
 	} );
 
@@ -241,6 +242,92 @@ describe( 'apiFetch', () => {
 		).rejects.toBe( mockResponse );
 	} );
 
+	it( 'should refetch after successful nonce refresh', async () => {
+		apiFetch.nonceMiddleware =
+			apiFetch.createNonceMiddleware( 'old-nonce' );
+		apiFetch.use( apiFetch.nonceMiddleware );
+		apiFetch.nonceEndpoint = '/rest-nonce';
+
+		globalThis.fetch.mockImplementation( async ( path, options ) => {
+			if ( path.startsWith( '/random' ) ) {
+				if ( options?.headers[ 'X-WP-Nonce' ] === 'new-nonce' ) {
+					return {
+						ok: true,
+						status: 200,
+						json: async () => ( { code: 'success' } ),
+					};
+				}
+
+				return {
+					ok: false,
+					status: 403,
+					json: async () => ( { code: 'rest_cookie_invalid_nonce' } ),
+				};
+			}
+
+			if ( path.startsWith( '/rest-nonce' ) ) {
+				return {
+					ok: true,
+					status: 200,
+					text: async () => 'new-nonce',
+				};
+			}
+
+			return {
+				ok: false,
+				status: 404,
+				json: async () => ( { code: 'rest_no_route' } ),
+			};
+		} );
+
+		await expect( apiFetch( { path: '/random' } ) ).resolves.toEqual( {
+			code: 'success',
+		} );
+	} );
+
+	it( 'should fail with rest_cookie_invalid_nonce after failed nonce refresh', async () => {
+		apiFetch.nonceMiddleware =
+			apiFetch.createNonceMiddleware( 'old-nonce' );
+		apiFetch.use( apiFetch.nonceMiddleware );
+		apiFetch.nonceEndpoint = '/rest-nonce';
+
+		globalThis.fetch.mockImplementation( async ( path, options ) => {
+			if ( path.startsWith( '/random' ) ) {
+				if ( options?.headers[ 'X-WP-Nonce' ] === 'new-nonce' ) {
+					return {
+						ok: true,
+						status: 200,
+						json: async () => ( { code: 'success' } ),
+					};
+				}
+
+				return {
+					ok: false,
+					status: 403,
+					json: async () => ( { code: 'rest_cookie_invalid_nonce' } ),
+				};
+			}
+
+			if ( path.startsWith( '/rest-nonce' ) ) {
+				return {
+					ok: false,
+					status: 400,
+					text: async () => '0',
+				};
+			}
+
+			return {
+				ok: false,
+				status: 404,
+				json: async () => ( { code: 'rest_no_route' } ),
+			};
+		} );
+
+		await expect( apiFetch( { path: '/random' } ) ).rejects.toEqual( {
+			code: 'rest_cookie_invalid_nonce',
+		} );
+	} );
+
 	it( 'should not use the default fetch handler when using a custom fetch handler', async () => {
 		const customFetchHandler = jest.fn();
 
@@ -256,13 +343,8 @@ describe( 'apiFetch', () => {
 	} );
 
 	it( 'should run the last-registered user-defined middleware first', async () => {
-		// This could potentially impact other tests in that a lingering
-		// middleware is left. For the purposes of this test, it is sufficient
-		// to ensure that the last-registered middleware receives the original
-		// options object. It also assumes that some built-in middleware would
-		// either mutate or clone the original options if the extra middleware
-		// had been pushed to the stack.
-		expect.assertions( 1 );
+		// The test assumes that some built-in middleware will either mutate or clone
+		// the original options if the extra middleware had been pushed to the stack.
 
 		const expectedOptions = {};
 
@@ -272,6 +354,9 @@ describe( 'apiFetch', () => {
 			return next( actualOptions );
 		} );
 
+		// Set a custom fetch handler to avoid using the default fetch handler.
+		apiFetch.setFetchHandler( jest.fn() );
+
 		await apiFetch( expectedOptions );
 	} );
 } );

packages/api-fetch/src/utils/response.ts

@@ -3,47 +3,23 @@
  */
 import { __ } from '@wordpress/i18n';
 
-/**
- * Parses the apiFetch response.
- *
- * @param response
- * @param shouldParseResponse
- *
- * @return Parsed response.
- */
-const parseResponse = ( response: Response, shouldParseResponse = true ) => {
-	if ( shouldParseResponse ) {
-		if ( response.status === 204 ) {
-			return null;
-		}
-
-		return response.json ? response.json() : Promise.reject( response );
-	}
-
-	return response;
-};
-
 /**
  * Calls the `json` function on the Response, throwing an error if the response
  * doesn't have a json function or if parsing the json itself fails.
  *
  * @param response
  * @return Parsed response.
  */
-const parseJsonAndNormalizeError = ( response: Response ) => {
-	const invalidJsonError = {
-		code: 'invalid_json',
-		message: __( 'The response is not a valid JSON response.' ),
-	};
-
-	if ( ! response || ! response.json ) {
-		throw invalidJsonError;
+async function parseJsonAndNormalizeError( response: Response ) {
+	try {
+		return await response.json();
+	} catch {
+		throw {
+			code: 'invalid_json',
+			message: __( 'The response is not a valid JSON response.' ),
+		};
 	}
-
-	return response.json().catch( () => {
-		throw invalidJsonError;
-	} );
-};
+}
 
 /**
  * Parses the apiFetch response properly and normalize response errors.
@@ -53,36 +29,36 @@ const parseJsonAndNormalizeError = ( response: Response ) => {
  *
  * @return Parsed response.
  */
-export const parseResponseAndNormalizeError = (
+export async function parseResponseAndNormalizeError(
 	response: Response,
 	shouldParseResponse = true
-) => {
-	return Promise.resolve(
-		parseResponse( response, shouldParseResponse )
-	).catch( ( res ) => parseAndThrowError( res, shouldParseResponse ) );
-};
+) {
+	if ( ! shouldParseResponse ) {
+		return response;
+	}
+
+	if ( response.status === 204 ) {
+		return null;
+	}
+
+	return await parseJsonAndNormalizeError( response );
+}
 
 /**
  * Parses a response, throwing an error if parsing the response fails.
  *
  * @param response
  * @param shouldParseResponse
- * @return Parsed response.
+ * @return Never returns, always throws.
  */
-export function parseAndThrowError(
+export async function parseAndThrowError(
 	response: Response,
 	shouldParseResponse = true
 ) {
 	if ( ! shouldParseResponse ) {
 		throw response;
 	}
 
-	return parseJsonAndNormalizeError( response ).then( ( error ) => {
-		const unknownError = {
-			code: 'unknown_error',
-			message: __( 'An unknown error occurred.' ),
-		};
-
-		throw error || unknownError;
-	} );
+	// Parse the response JSON and throw it as an error.
+	throw await parseJsonAndNormalizeError( response );
 }