Improve the error thrown when a bundle fails to load (#51070)

Summary:
Pull Request resolved: #51070

Changelog: [General][Internal][Breaking] When a bundle is failing to load throw an error of special type.

### Breaking
`didCompleteNetworkResponse` now throws an Error instead of throwing a string.

In dev, we use [lazy bundling](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0605-lazy-bundling.md#__loadbundleasync-in-metro) which allows us to re-build only the parts of the app that are changed.

However the function that loads these lazy bundles in `loadBundleFromServer.js` for requests that reach `didCompleteNetworkResponse` with an error **throws a string representing the error message** which makes debugging very inconvenient because it lacks stack trace and error type. Throwing strings is not a standard practice. We better throw an error in these cases so it can be handled better.

Why a special type of error?
* As you can see in the "after", in the test plan below, even with a stack trace, it might be hard to know where this error originates from
* Also so extra information can be added on it. Currently adding "url"
* Also to support error handling based on the type of error

Reviewed By: hoxyq

Differential Revision: D73925027

fbshipit-source-id: a0f98d283ec8842696f1b87864fb63cb30c0c028

Author: vzaidman

packages/react-native/Libraries/Core/Devtools/__tests__/loadBundleFromServer-test.js

@@ -62,7 +62,7 @@ jest.mock('../../../Network/RCTNetworking', () => ({
       if (name === 'didReceiveNetworkData') {
         setImmediate(() => fn([1, mockDataResponse]));
       } else if (name === 'didCompleteNetworkResponse') {
-        setImmediate(() => fn([1, null]));
+        setImmediate(() => fn([1, mockRequestError]));
       } else if (name === 'didReceiveNetworkResponse') {
         setImmediate(() => fn([1, null, mockHeaders]));
       }
@@ -72,9 +72,14 @@ jest.mock('../../../Network/RCTNetworking', () => ({
 }));
 
 let loadBundleFromServer: (bundlePathAndQuery: string) => Promise<void>;
+const {
+  LoadBundleFromServerError,
+  LoadBundleFromServerRequestError,
+} = require('../loadBundleFromServer');
 
 let mockHeaders: {'Content-Type': string};
 let mockDataResponse;
+let mockRequestError = null;
 
 beforeEach(() => {
   jest.resetModules();
@@ -85,15 +90,35 @@ beforeEach(() => {
 test('loadBundleFromServer will throw for JSON responses', async () => {
   mockHeaders = {'Content-Type': 'application/json'};
   mockDataResponse = JSON.stringify({message: 'Error thrown from Metro'});
+  mockRequestError = null;
+
+  try {
+    await loadBundleFromServer('/Fail.bundle?platform=ios');
+  } catch (e) {
+    expect(e).toBeInstanceOf(LoadBundleFromServerError);
+    expect(e.message).toBe('Could not load bundle');
+    expect(e.cause).toBe('Error thrown from Metro');
+  }
+});
 
-  await expect(
-    loadBundleFromServer('/Fail.bundle?platform=ios'),
-  ).rejects.toThrow('Error thrown from Metro');
+test('loadBundleFromServer will throw LoadBundleFromServerError for request errors', async () => {
+  mockHeaders = {'Content-Type': 'application/json'};
+  mockDataResponse = '';
+  mockRequestError = 'Some error';
+
+  try {
+    await loadBundleFromServer('/Fail.bundle?platform=ios');
+  } catch (e) {
+    expect(e).toBeInstanceOf(LoadBundleFromServerRequestError);
+    expect(e.message).toBe('Could not load bundle');
+    expect(e.cause).toBe('Some error');
+  }
 });
 
 test('loadBundleFromServer will request a bundle if import bundles are available', async () => {
   mockDataResponse = '"code";';
   mockHeaders = {'Content-Type': 'application/javascript'};
+  mockRequestError = null;
 
   await loadBundleFromServer(
     '/Banana.bundle?platform=ios&dev=true&minify=false&unusedExtraParam=42&modulesOnly=true&runModule=false',
@@ -138,6 +163,7 @@ test('loadBundleFromServer will request a bundle if import bundles are available
 test('shows and hides the loading view around a request', async () => {
   mockDataResponse = '"code";';
   mockHeaders = {'Content-Type': 'application/javascript'};
+  mockRequestError = null;
 
   const promise = loadBundleFromServer(
     '/Banana.bundle?platform=ios&dev=true&minify=false&unusedExtraParam=42&modulesOnly=true&runModule=false',
@@ -157,6 +183,7 @@ test('shows and hides the loading view around a request', async () => {
 test('shows and hides the loading view around concurrent requests', async () => {
   mockDataResponse = '"code";';
   mockHeaders = {'Content-Type': 'application/javascript'};
+  mockRequestError = null;
 
   const promise1 = loadBundleFromServer(
     '/Banana.bundle?platform=ios&dev=true&minify=false&unusedExtraParam=42&modulesOnly=true&runModule=false',
@@ -178,13 +205,15 @@ test('shows and hides the loading view around concurrent requests', async () =>
 test('loadBundleFromServer does not cache errors', async () => {
   mockHeaders = {'Content-Type': 'application/json'};
   mockDataResponse = JSON.stringify({message: 'Error thrown from Metro'});
+  mockRequestError = null;
 
   await expect(
     loadBundleFromServer('/Fail.bundle?platform=ios'),
   ).rejects.toThrow();
 
   mockDataResponse = '"code";';
   mockHeaders = {'Content-Type': 'application/javascript'};
+  mockRequestError = null;
 
   await expect(
     loadBundleFromServer('/Fail.bundle?platform=ios'),
@@ -194,6 +223,7 @@ test('loadBundleFromServer does not cache errors', async () => {
 test('loadBundleFromServer caches successful fetches', async () => {
   mockDataResponse = '"code";';
   mockHeaders = {'Content-Type': 'application/javascript'};
+  mockRequestError = null;
 
   const promise1 = loadBundleFromServer(
     '/Banana.bundle?platform=ios&dev=true&minify=false&unusedExtraParam=42&modulesOnly=true&runModule=false',

packages/react-native/Libraries/Core/Devtools/loadBundleFromServer.js

@@ -20,6 +20,34 @@ let pendingRequests = 0;
 
 const cachedPromisesByUrl = new Map<string, Promise<void>>();
 
+export class LoadBundleFromServerError extends Error {
+  url: string;
+  isTimeout: boolean;
+  constructor(
+    message: string,
+    url: string,
+    isTimeout: boolean,
+    options?: {cause: mixed, ...},
+  ): void {
+    super(message, options);
+    this.url = url;
+    this.isTimeout = isTimeout;
+    this.name = 'LoadBundleFromServerError';
+  }
+}
+
+export class LoadBundleFromServerRequestError extends LoadBundleFromServerError {
+  constructor(
+    message: string,
+    url: string,
+    isTimeout: boolean,
+    options?: {cause: mixed, ...},
+  ): void {
+    super(message, url, isTimeout, options);
+    this.name = 'LoadBundleFromServerRequestError';
+  }
+}
+
 function asyncRequest(
   url: string,
 ): Promise<{body: string, headers: {[string]: string}}> {
@@ -62,10 +90,19 @@ function asyncRequest(
       );
       completeListener = Networking.addListener(
         'didCompleteNetworkResponse',
-        ([requestId, error]) => {
+        ([requestId, errorMessage, isTimeout]) => {
           if (requestId === id) {
-            if (error) {
-              reject(error);
+            if (errorMessage) {
+              reject(
+                new LoadBundleFromServerRequestError(
+                  'Could not load bundle',
+                  url,
+                  isTimeout,
+                  {
+                    cause: errorMessage,
+                  },
+                ),
+              );
             } else {
               //$FlowFixMe[incompatible-call]
               resolve({body: responseText, headers});
@@ -122,9 +159,15 @@ export default function loadBundleFromServer(
         headers['Content-Type'].indexOf('application/json') >= 0
       ) {
         // Errors are returned as JSON.
-        throw new Error(
-          JSON.parse(body).message ||
-            `Unknown error fetching '${bundlePathAndQuery}'`,
+        throw new LoadBundleFromServerError(
+          'Could not load bundle',
+          bundlePathAndQuery,
+          false, // isTimeout
+          {
+            cause:
+              JSON.parse(body).message ||
+              `Unknown error fetching '${bundlePathAndQuery}'`,
+          },
         );
       }
 

packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap

@@ -3843,7 +3843,27 @@ declare export default function getDevServer(): DevServerInfo;
 `;
 
 exports[`public API should not change unintentionally Libraries/Core/Devtools/loadBundleFromServer.js 1`] = `
-"declare export default function loadBundleFromServer(
+"declare export class LoadBundleFromServerError extends Error {
+  url: string;
+  isTimeout: boolean;
+  constructor(
+    message: string,
+    url: string,
+    isTimeout: boolean,
+    options?: { cause: mixed, ... }
+  ): void;
+}
+declare export class LoadBundleFromServerRequestError
+  extends LoadBundleFromServerError
+{
+  constructor(
+    message: string,
+    url: string,
+    isTimeout: boolean,
+    options?: { cause: mixed, ... }
+  ): void;
+}
+declare export default function loadBundleFromServer(
   bundlePathAndQuery: string
 ): Promise<void>;
 "