Skip to content

Commit 4c13990

Browse files
eps1loneps1lon
committed
[Fiber] Throw special error if rejected Promises are incorrectly instrumented
1 parent 142cfde commit 4c13990

5 files changed

Lines changed: 185 additions & 2 deletions

File tree

‎packages/react-reconciler/src/ReactFiberThenable.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,12 +192,30 @@ export function trackUsedThenable<T>(
192192
// a listener that will update its status and result when it resolves.
193193
switch (thenable.status) {
194194
case 'fulfilled': {
195+
// This could be a bad instrumentation that doesn't set .value.
196+
// We're not type-checking since this is a hot path where you can
197+
// track down easily when something becomes `undefined` unexpectedly.
195198
const fulfilledValue: T = thenable.value;
196199
return fulfilledValue;
197200
}
198201
case 'rejected': {
199202
const rejectedError = thenable.reason;
200203
checkIfUseWrappedInAsyncCatch(rejectedError);
204+
205+
// Rejected Promises are rarer so we're doing an extra type-check in
206+
// case of a bad instrumentation that doesn't set .reason
207+
// If we end up throwing `undefined` it becomes hard to track down
208+
// where that throw originated because no callstack would exist.
209+
// React would still have a Component stack but that could only be used
210+
// as an approximation.
211+
if (rejectedError === undefined && !('reason' in thenable)) {
212+
throw new Error(
213+
'A rejected Promise was passed to React without a `reason` property. ' +
214+
'React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. ' +
215+
"Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`.",
216+
);
217+
}
218+
201219
throw rejectedError;
202220
}
203221
default: {

‎packages/react-reconciler/src/__tests__/ReactUse-test.js‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,30 @@ describe('ReactUse', () => {
8181
});
8282
}
8383

84+
function normalizeCodeLocInfo(str) {
85+
return (
86+
str &&
87+
str.replace(
88+
/^ +(?:at|in) ([\S]+)([^\n]*)(\n?)/gm,
89+
function (m, name, location, eol) {
90+
if (location.indexOf(__filename) === -1) {
91+
// ignore frames from library code
92+
return '';
93+
}
94+
return (
95+
' in ' +
96+
name +
97+
(/:\d+:\d+/.test(m)
98+
? ' ' +
99+
location.replace(__dirname, '~').replace(/:\d+:\d+/, ':*:*')
100+
: '') +
101+
eol
102+
);
103+
},
104+
)
105+
);
106+
}
107+
84108
function Text({text}) {
85109
Scheduler.log(text);
86110
return text;
@@ -2189,4 +2213,108 @@ describe('ReactUse', () => {
21892213
expect(root).toMatchRenderedOutput('Updated');
21902214
},
21912215
);
2216+
2217+
it('throws a descriptive error when a rejected promise without a reason property is passed to use()', async () => {
2218+
const badThenable = Promise.resolve();
2219+
// Simulate bad instrumentation: status is 'rejected' but the reason is not
2220+
// in the `reason` property as intended.
2221+
badThenable.status = 'rejected';
2222+
badThenable.error = new Error('Something went wrong');
2223+
2224+
function Child() {
2225+
return use(badThenable);
2226+
}
2227+
2228+
function App({
2229+
// Intentionally destrucutring a prop here so that our production error
2230+
// stack trick is triggered at the beginning of the function
2231+
prop,
2232+
}) {
2233+
return <Child />;
2234+
}
2235+
2236+
const uncaughtErrors = [];
2237+
const root = ReactNoop.createRoot({
2238+
onUncaughtError(error, errorInfo) {
2239+
uncaughtErrors.push({
2240+
callStack: normalizeCodeLocInfo(error.stack),
2241+
ownerStack: React.captureOwnerStack
2242+
? normalizeCodeLocInfo(React.captureOwnerStack())
2243+
: null,
2244+
componentStack: normalizeCodeLocInfo(errorInfo.componentStack),
2245+
});
2246+
},
2247+
});
2248+
2249+
await act(() => {
2250+
root.render(<App />);
2251+
});
2252+
2253+
expect(uncaughtErrors).toEqual([
2254+
{
2255+
callStack:
2256+
'Error: A rejected Promise was passed to React without a `reason` property. ' +
2257+
'React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. ' +
2258+
"Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`." +
2259+
'\n in Child (~/ReactUse-test.js:*:*)' +
2260+
'\n in Object.<anonymous> (~/ReactUse-test.js:*:*)',
2261+
componentStack:
2262+
'\n in Child (~/ReactUse-test.js:*:*)' +
2263+
'\n in App (~/ReactUse-test.js:*:*)',
2264+
ownerStack: __DEV__ ? '\n in App (~/ReactUse-test.js:*:*)' : null,
2265+
},
2266+
]);
2267+
});
2268+
2269+
it('throws a descriptive error when a rejected promise without a reason property is used as a child', async () => {
2270+
const badThenable = Promise.resolve();
2271+
// Simulate bad instrumentation: status is 'rejected' but the reason is not
2272+
// in the `reason` property as intended.
2273+
badThenable.status = 'rejected';
2274+
badThenable.error = new Error('Something went wrong');
2275+
2276+
function Child() {
2277+
return <div>{badThenable}</div>;
2278+
}
2279+
2280+
function App({
2281+
// Intentionally destrucutring a prop here so that our production error
2282+
// stack trick is triggered at the beginning of the function
2283+
prop,
2284+
}) {
2285+
return <Child />;
2286+
}
2287+
2288+
const uncaughtErrors = [];
2289+
const root = ReactNoop.createRoot({
2290+
onUncaughtError(error, errorInfo) {
2291+
uncaughtErrors.push({
2292+
callStack: normalizeCodeLocInfo(error.stack),
2293+
ownerStack: React.captureOwnerStack
2294+
? normalizeCodeLocInfo(React.captureOwnerStack())
2295+
: null,
2296+
componentStack: normalizeCodeLocInfo(errorInfo.componentStack),
2297+
});
2298+
},
2299+
});
2300+
2301+
await act(() => {
2302+
root.render(<App />);
2303+
});
2304+
2305+
expect(uncaughtErrors).toEqual([
2306+
{
2307+
callStack:
2308+
'Error: A rejected Promise was passed to React without a `reason` property. ' +
2309+
'React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. ' +
2310+
"Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`." +
2311+
'\n in Object.<anonymous> (~/ReactUse-test.js:*:*)',
2312+
componentStack: '\n in App (~/ReactUse-test.js:*:*)',
2313+
ownerStack: __DEV__
2314+
? // TODO: Should start in Child since that's the owner.
2315+
'\n in App (~/ReactUse-test.js:*:*)'
2316+
: null,
2317+
},
2318+
]);
2319+
});
21922320
});

‎packages/react-server/src/ReactFizzThenable.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,29 @@ export function trackUsedThenable<T>(
6969
// a listener that will update its status and result when it resolves.
7070
switch (thenable.status) {
7171
case 'fulfilled': {
72+
// This could be a bad instrumentation that doesn't set .value.
73+
// We're not type-checking since this is a hot path where you can
74+
// track down easily when something becomes `undefined` unexpectedly.
7275
const fulfilledValue: T = thenable.value;
7376
return fulfilledValue;
7477
}
7578
case 'rejected': {
7679
const rejectedError = thenable.reason;
80+
81+
// Rejected Promises are rarer so we're doing an extra type-check in
82+
// case of a bad instrumentation that doesn't set .reason
83+
// If we end up throwing `undefined` it becomes hard to track down
84+
// where that throw originated because no callstack would exist.
85+
// React would still have a Component stack but that could only be used
86+
// as an approximation.
87+
if (rejectedError === undefined && !('reason' in thenable)) {
88+
throw new Error(
89+
'A rejected Promise was passed to React without a `reason` property. ' +
90+
'React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. ' +
91+
"Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`.",
92+
);
93+
}
94+
7795
throw rejectedError;
7896
}
7997
default: {

‎packages/react-server/src/ReactFlightThenable.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,29 @@ export function trackUsedThenable<T>(
7777
// a listener that will update its status and result when it resolves.
7878
switch (thenable.status) {
7979
case 'fulfilled': {
80+
// This could be a bad instrumentation that doesn't set .value.
81+
// We're not type-checking since this is a hot path where you can
82+
// track down easily when something becomes `undefined` unexpectedly.
8083
const fulfilledValue: T = thenable.value;
8184
return fulfilledValue;
8285
}
8386
case 'rejected': {
8487
const rejectedError = thenable.reason;
88+
89+
// Rejected Promises are rarer so we're doing an extra type-check in
90+
// case of a bad instrumentation that doesn't set .reason
91+
// If we end up throwing `undefined` it becomes hard to track down
92+
// where that throw originated because no callstack would exist.
93+
// React would still have a Component stack but that could only be used
94+
// as an approximation.
95+
if (rejectedError === undefined && !('reason' in thenable)) {
96+
throw new Error(
97+
'A rejected Promise was passed to React without a `reason` property. ' +
98+
'React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. ' +
99+
"Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`.",
100+
);
101+
}
102+
85103
throw rejectedError;
86104
}
87105
default: {

‎scripts/error-codes/codes.json‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -582,5 +582,6 @@
582582
"594": "Cannot read Symbol exports. Only named exports are supported on a client module imported on the server.",
583583
"595": "Attempted to call %s() from the server but %s is on the client. It's not possible to invoke a client function from the server, it can only be rendered as a Component or passed to props of a Client Component.",
584584
"596": "Could not find the module \"%s\" in the React Client Manifest. This is probably a bug in the React Server Components bundler.",
585-
"597": "The module \"%s\" is marked as an async ESM module but was loaded as a CJS proxy. This is probably a bug in the React Server Components bundler."
586-
}
585+
"597": "The module \"%s\" is marked as an async ESM module but was loaded as a CJS proxy. This is probably a bug in the React Server Components bundler.",
586+
"598": "A rejected Promise was passed to React without a `reason` property. React threw a generic error from where the Promise was used to assist in identifying the problematic Promise. Make sure that instrumented Promises correctly set the `reason` property when setting `status` to `'rejected'`."
587+
}

0 commit comments

Comments
 (0)