Skip to content

Commit

Permalink
Rethrow cached module errors without wrapping
Browse files Browse the repository at this point in the history
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
  • Loading branch information
motiz88 authored and facebook-github-bot committed Nov 30, 2022
1 parent 4793b5e commit 032c4a1
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 21 deletions.
58 changes: 48 additions & 10 deletions packages/metro-runtime/src/polyfills/__tests__/require-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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,
});
12 changes: 1 addition & 11 deletions packages/metro-runtime/src/polyfills/require.js
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ function loadModuleImplementation(
}

if (module.hasError) {
throw moduleThrewError(moduleId, module.error);
throw module.error;
}

if (__DEV__) {
Expand Down Expand Up @@ -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 = {
Expand Down

0 comments on commit 032c4a1

Please sign in to comment.