From 032c4a1e03f3f696c8526c7771621d8e312e3f61 Mon Sep 17 00:00:00 2001
From: Moti Zilberman <moti@meta.com>
Date: Wed, 30 Nov 2022 06:04:58 -0800
Subject: [PATCH] Rethrow cached module errors without wrapping

Summary:
Changelog:
* **[Fix]:** `metro-runtime`: Rethrow cached module errors without wrapping

Metro's `require()` and `import` implementation caches errors thrown during module evaluation, and doesn't re-evaluate modules (except for HMR). Here, we keep this behaviour, but will now always throw the original (cached) error object instead of wrapping it in a new one. This is so that we can show information about the original error in UIs like LogBox.

## Side note: Why cache errors?

Caching errors (without wrapping) is what Webpack and Node have aligned on for ES modules, though FWIW their legacy `require` runtimes behave quite differently:

* Node re-evaluates throwing modules on every `require`, so assuming a module that throws deterministically, the error object is new every time. (This is what I was going to propose before doing the rest of this research.)
* In Webpack only the first `require` throws and subsequent calls fail silently)

Given this alignment in the ESM space, the fact that Metro mixes ESM and CommonJS in a single module registry, and how long the caching behaviour has existed in Metro - it's best *not* to add re-evaluation semantics to Metro's `require()` at this point.

## Should we use [`Error.prototype.cause`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause) ?

We could in theory add the cached error as the `cause` of the new error, which would allow error handlers to keep track of both the "outer" and "inner" stack traces. However, this would require more nontrivial changes (particularly to React Native's LogBox and ExceptionsManager) to be truly useful, so I'm not doing it here.

Reviewed By: huntie

Differential Revision: D41475884

fbshipit-source-id: 0a1da0118a0ade29d55803dee5e7d00f6d3ad728
---
 .../src/polyfills/__tests__/require-test.js   | 58 +++++++++++++++----
 .../metro-runtime/src/polyfills/require.js    | 12 +---
 2 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/packages/metro-runtime/src/polyfills/__tests__/require-test.js b/packages/metro-runtime/src/polyfills/__tests__/require-test.js
index d2c703c879..3888dbc871 100644
--- a/packages/metro-runtime/src/polyfills/__tests__/require-test.js
+++ b/packages/metro-runtime/src/polyfills/__tests__/require-test.js
@@ -505,22 +505,22 @@ describe('require', () => {
     it('throws an error when a module throws an error', () => {
       createModuleSystem(moduleSystem, false, '');
 
-      createModule(
-        moduleSystem,
-        0,
-        'foo.js',
+      const error = new Error('foo!');
+      const factory = jest.fn(
         (global, require, importDefault, importAll, module) => {
-          throw new Error('foo!');
+          throw error;
         },
       );
+      createModule(moduleSystem, 0, 'foo.js', factory);
 
       // First time it throws the original error.
-      expect(() => moduleSystem.__r(0)).toThrow('foo!');
+      expect(() => moduleSystem.__r(0)).toThrowStrictEquals(error);
 
-      // Afterwards it throws a wrapped error (the module is not reevaluated).
-      expect(() => moduleSystem.__r(0)).toThrow(
-        'Requiring module "0", which threw an exception: Error: foo!',
-      );
+      // Afterwards it throws the exact same error.
+      expect(() => moduleSystem.__r(0)).toThrowStrictEquals(error);
+
+      // The module is not reevaluated.
+      expect(factory).toHaveBeenCalledTimes(1);
     });
 
     it('can make use of the dependencyMap correctly', () => {
@@ -2819,3 +2819,41 @@ describe('require', () => {
     });
   });
 });
+
+function toThrowStrictEquals(received, expected) {
+  let thrown = null;
+  try {
+    received();
+  } catch (e) {
+    thrown = {value: e};
+  }
+  const pass = thrown && thrown.value === expected;
+  if (pass) {
+    return {
+      message: () =>
+        `expected function not to throw ${this.utils.printExpected(
+          expected,
+        )} but it did`,
+      pass: true,
+    };
+  } else {
+    return {
+      message: () => {
+        if (thrown) {
+          return `expected function to throw ${this.utils.printExpected(
+            expected,
+          )} but received ${this.utils.printReceived(thrown.value)}`;
+        } else {
+          return `expected function to throw ${this.utils.printExpected(
+            expected,
+          )} but it did not throw`;
+        }
+      },
+      pass: false,
+    };
+  }
+}
+
+expect.extend({
+  toThrowStrictEquals,
+});
diff --git a/packages/metro-runtime/src/polyfills/require.js b/packages/metro-runtime/src/polyfills/require.js
index fa0973fd59..5f1db314b5 100644
--- a/packages/metro-runtime/src/polyfills/require.js
+++ b/packages/metro-runtime/src/polyfills/require.js
@@ -398,7 +398,7 @@ function loadModuleImplementation(
   }
 
   if (module.hasError) {
-    throw moduleThrewError(moduleId, module.error);
+    throw module.error;
   }
 
   if (__DEV__) {
@@ -498,16 +498,6 @@ function unknownModuleError(id: ModuleID): Error {
   return Error(message);
 }
 
-function moduleThrewError(id: ModuleID, error: any): Error {
-  const displayName = (__DEV__ && modules[id] && modules[id].verboseName) || id;
-  return Error(
-    'Requiring module "' +
-      displayName +
-      '", which threw an exception: ' +
-      error,
-  );
-}
-
 if (__DEV__) {
   // $FlowFixMe[prop-missing]
   metroRequire.Systrace = {