Skip to content

Commit 8fc5763

Browse files
authored
fix[describeClassComponentFrame]: invoke constructor with new keyword (facebook#36455)
For JavaScript runtimes that do not have [`Reflect`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect) supported, we had a fall back that was calling the constructor with overridden `this` context via ``` fn.apply(Fake.prototype); ``` In ES6, it is required to call constructor only with the `new` keyword, otherwise the runtime is expected to throw a corresponding TypeError: ``` TypeError: Class constructor <> cannot be invoked without 'new' ``` We've observed this error in Hermes runtime, but this is applicable to V8 or any other runtime. The only reason why V8 wasn't affected is because it implemented Reflect APIs. Instead of the incorrect call, we will fall back to calling `new fn()`, but with a temporary patched prototype of the class, which would make a trap out of the setter for `props` object. We use the same approach when `Reflect` APIs are available, but instead of modifying the prototype, we pass the fake context: https://github.com/facebook/react/blob/d5736f098edee62c44f27b053e6e48f5fa443803/packages/shared/ReactComponentStackFrame.js#L129-L148 --- See tests implemented. Without the changes, the test would fail with the `TypeError` mentioned above.
1 parent d5736f0 commit 8fc5763

4 files changed

Lines changed: 105 additions & 16 deletions

File tree

packages/react-devtools-shared/src/backend/shared/DevToolsComponentStackFrame.js

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,33 @@ export function describeNativeComponentFrame(
129129
} catch (x) {
130130
control = x;
131131
}
132-
// $FlowFixMe[prop-missing] found when upgrading Flow
133-
fn.call(Fake.prototype);
132+
133+
let prototypeModified = false;
134+
let prevProps;
135+
try {
136+
prevProps = Object.getOwnPropertyDescriptor(
137+
fn.prototype,
138+
'props',
139+
);
140+
Object.defineProperty(fn.prototype, 'props', {
141+
configurable: true,
142+
set() {
143+
throw Error();
144+
},
145+
});
146+
prototypeModified = true;
147+
148+
// eslint-disable-next-line no-new
149+
new fn();
150+
} finally {
151+
if (prototypeModified) {
152+
if (prevProps !== undefined) {
153+
Object.defineProperty(fn.prototype, 'props', prevProps);
154+
} else {
155+
delete fn.prototype.props;
156+
}
157+
}
158+
}
134159
}
135160
} else {
136161
try {

packages/react-reconciler/src/__tests__/ReactErrorStacks-test.js

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,4 +342,41 @@ describe('ReactFragment', () => {
342342
__DEV__ ? componentStack(['SomethingThatErrors']) : null,
343343
]);
344344
});
345+
346+
it('captures class component stacks via the no-Reflect.construct fallback', async () => {
347+
class FailingClassComponent extends React.Component {
348+
render() {
349+
throw new Error('uh oh');
350+
}
351+
}
352+
353+
const root = ReactNoop.createRoot({onCaughtError});
354+
355+
const originalReflectConstruct = Reflect.construct;
356+
delete Reflect.construct;
357+
358+
try {
359+
root.render(
360+
<CatchingBoundary>
361+
<FailingClassComponent />
362+
</CatchingBoundary>,
363+
);
364+
await waitForAll([]);
365+
} finally {
366+
Object.defineProperty(Reflect, 'construct', {
367+
value: originalReflectConstruct,
368+
});
369+
}
370+
371+
expect(rootCaughtErrors).toEqual([
372+
'uh oh',
373+
componentStack(['FailingClassComponent', 'CatchingBoundary']),
374+
__DEV__ ? componentStack(['FailingClassComponent']) : null,
375+
]);
376+
377+
// The throwing trap should be reset, we use props object for it
378+
expect(
379+
Object.getOwnPropertyDescriptor(FailingClassComponent.prototype, 'props'),
380+
).toBeUndefined();
381+
});
345382
});

packages/shared/ReactComponentStackFrame.js

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,33 @@ export function describeNativeComponentFrame(
152152
} catch (x) {
153153
control = x;
154154
}
155-
// $FlowFixMe[prop-missing] found when upgrading Flow
156-
fn.call(Fake.prototype);
155+
156+
let prototypeModified = false;
157+
let prevProps;
158+
try {
159+
prevProps = Object.getOwnPropertyDescriptor(
160+
fn.prototype,
161+
'props',
162+
);
163+
Object.defineProperty(fn.prototype, 'props', {
164+
configurable: true,
165+
set() {
166+
throw Error();
167+
},
168+
});
169+
prototypeModified = true;
170+
171+
// eslint-disable-next-line no-new
172+
new fn();
173+
} finally {
174+
if (prototypeModified) {
175+
if (prevProps !== undefined) {
176+
Object.defineProperty(fn.prototype, 'props', prevProps);
177+
} else {
178+
delete fn.prototype.props;
179+
}
180+
}
181+
}
157182
}
158183
} else {
159184
try {

scripts/jest/setupTests.js

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,13 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {
115115
return format.replace(/%s/g, () => args[argIndex++]);
116116
};
117117
const OriginalError = global.Error;
118+
// Cache Reflect methods so the proxies keep working even if a test
119+
// temporarily deletes or overrides them (e.g. to exercise no-Reflect
120+
// fallback paths in the React source).
121+
const ReflectApply = Reflect.apply;
122+
const ReflectConstruct = Reflect.construct;
123+
const ReflectGet = Reflect.get;
124+
const ReflectSet = Reflect.set;
118125
// V8's Error.captureStackTrace (used in Jest) fails if the error object is
119126
// a Proxy, so we need to pass it the unproxied instance.
120127
const originalErrorInstances = new WeakMap();
@@ -131,42 +138,37 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {
131138
const proxy = new Proxy(error, {
132139
set(target, key, value, receiver) {
133140
if (key === 'message') {
134-
return Reflect.set(
135-
target,
136-
key,
137-
decodeErrorMessage(value),
138-
receiver
139-
);
141+
return ReflectSet(target, key, decodeErrorMessage(value), receiver);
140142
}
141-
return Reflect.set(target, key, value, receiver);
143+
return ReflectSet(target, key, value, receiver);
142144
},
143145
get(target, key, receiver) {
144146
if (key === 'stack') {
145147
// https://github.com/nodejs/node/issues/60862
146-
return Reflect.get(target, key);
148+
return ReflectGet(target, key);
147149
}
148-
return Reflect.get(target, key, receiver);
150+
return ReflectGet(target, key, receiver);
149151
},
150152
});
151153
originalErrorInstances.set(proxy, error);
152154
return proxy;
153155
};
154156
const ErrorProxy = new Proxy(OriginalError, {
155157
apply(target, thisArg, argumentsList) {
156-
const error = Reflect.apply(target, thisArg, argumentsList);
158+
const error = ReflectApply(target, thisArg, argumentsList);
157159
error.message = decodeErrorMessage(error.message);
158160
return proxyErrorInstance(error);
159161
},
160162
construct(target, argumentsList, newTarget) {
161-
const error = Reflect.construct(target, argumentsList, newTarget);
163+
const error = ReflectConstruct(target, argumentsList, newTarget);
162164
error.message = decodeErrorMessage(error.message);
163165
return proxyErrorInstance(error);
164166
},
165167
get(target, key, receiver) {
166168
if (key === 'captureStackTrace') {
167169
return captureStackTrace;
168170
}
169-
return Reflect.get(target, key, receiver);
171+
return ReflectGet(target, key, receiver);
170172
},
171173
});
172174
ErrorProxy.OriginalError = OriginalError;

0 commit comments

Comments
 (0)