Skip to content

Commit 3751326

Browse files
sebmarkbageAndyPengc12
authored andcommitted
Remove __self and __source location from elements (facebook#28265)
Along with all the places using it like the `_debugSource` on Fiber. This still lets them be passed into `createElement` (and JSX dev runtime) since those can still be used in existing already compiled code and we don't want that to start spreading to DOM attributes. We used to have a DEV mode that compiles the source location of JSX into the compiled output. This was nice because we could get the actual call site of the JSX (instead of just somewhere in the component). It had a bunch of issues though: - It only works with JSX. - The way this source location is compiled is different in all the pipelines along the way. It relies on this transform being first and the source location we want to extract but it doesn't get preserved along source maps and don't have a way to be connected to the source hosted by the source maps. Ideally it should just use the mechanism other source maps use. - Since it's expensive it only works in DEV so if it's used for component stacks it would vary between dev and prod. - It only captures the callsite of the JSX and not the stack between the component and that callsite. In the happy case it's in the component but not always. Instead, we have another zero-cost trick to extract the call site of each component lazily only if it's needed. This ensures that component stacks are the same in DEV and PROD. At the cost of worse line number information. The better way to get the JSX call site would be to get it from `new Error()` or `console.createTask()` inside the JSX runtime which can capture the whole stack in a consistent way with other source mappings. We might explore that in the future. This removes source location info from React DevTools and React Native Inspector. The "jump to source code" feature or inspection can be made lazy instead by invoking the lazy component stack frame generation. That way it can be made to work in prod too. The filtering based on file path is a bit trickier. When redesigned this UI should ideally also account for more than one stack frame. With this change the DEV only Babel transforms are effectively deprecated since they're not necessary for anything.
1 parent 290f308 commit 3751326

26 files changed

+66
-211
lines changed

packages/react-client/src/ReactFlightClient.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -475,18 +475,6 @@ function createElement(
475475
writable: true,
476476
value: true, // This element has already been validated on the server.
477477
});
478-
Object.defineProperty(element, '_self', {
479-
configurable: false,
480-
enumerable: false,
481-
writable: false,
482-
value: null,
483-
});
484-
Object.defineProperty(element, '_source', {
485-
configurable: false,
486-
enumerable: false,
487-
writable: false,
488-
value: null,
489-
});
490478
}
491479
return element;
492480
}

packages/react-devtools-inline/__tests__/__e2e__/components.test.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,15 @@ test.describe('Components', () => {
9292
? valueElement.value
9393
: valueElement.innerText;
9494

95-
return [name, value, source.innerText];
95+
return [name, value, source ? source.innerText : null];
9696
},
9797
{name: isEditableName, value: isEditableValue}
9898
);
9999

100100
expect(propName).toBe('label');
101101
expect(propValue).toBe('"one"');
102-
expect(sourceText).toMatch(/ListApp[a-zA-Z]*\.js/);
102+
expect(sourceText).toBe(null);
103+
// TODO: expect(sourceText).toMatch(/ListApp[a-zA-Z]*\.js/);
103104
});
104105

105106
test('should allow props to be edited', async () => {

packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,13 @@ describe('Store component filters', () => {
242242
]),
243243
);
244244

245-
expect(store).toMatchInlineSnapshot(`[root]`);
245+
// TODO: Filtering should work on component location.
246+
// expect(store).toMatchInlineSnapshot(`[root]`);
247+
expect(store).toMatchInlineSnapshot(`
248+
[root]
249+
▾ <Component>
250+
<div>
251+
`);
246252

247253
await actAsync(
248254
async () =>

packages/react-devtools-shared/src/backend/legacy/renderer.js

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -773,12 +773,10 @@ export function attach(
773773
let owners = null;
774774
let props = null;
775775
let state = null;
776-
let source = null;
777776

778777
const element = internalInstance._currentElement;
779778
if (element !== null) {
780779
props = element.props;
781-
source = element._source != null ? element._source : null;
782780

783781
let owner = element._owner;
784782
if (owner) {
@@ -851,9 +849,6 @@ export function attach(
851849
// List of owners
852850
owners,
853851

854-
// Location of component in source code.
855-
source,
856-
857852
rootType: null,
858853
rendererPackageName: null,
859854
rendererVersion: null,

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

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -958,7 +958,7 @@ export function attach(
958958

959959
// NOTICE Keep in sync with get*ForFiber methods
960960
function shouldFilterFiber(fiber: Fiber): boolean {
961-
const {_debugSource, tag, type, key} = fiber;
961+
const {tag, type, key} = fiber;
962962

963963
switch (tag) {
964964
case DehydratedSuspenseComponent:
@@ -1010,15 +1010,15 @@ export function attach(
10101010
}
10111011
}
10121012

1013-
if (_debugSource != null && hideElementsWithPaths.size > 0) {
1014-
const {fileName} = _debugSource;
1015-
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
1016-
for (const pathRegExp of hideElementsWithPaths) {
1017-
if (pathRegExp.test(fileName)) {
1018-
return true;
1019-
}
1020-
}
1021-
}
1013+
// TODO: Figure out a way to filter by path in the new model which has no debug info.
1014+
// if (hideElementsWithPaths.size > 0) {
1015+
// const {fileName} = ...;
1016+
// for (const pathRegExp of hideElementsWithPaths) {
1017+
// if (pathRegExp.test(fileName)) {
1018+
// return true;
1019+
// }
1020+
// }
1021+
// }
10221022

10231023
return false;
10241024
}
@@ -3132,7 +3132,6 @@ export function attach(
31323132

31333133
const {
31343134
_debugOwner,
3135-
_debugSource,
31363135
stateNode,
31373136
key,
31383137
memoizedProps,
@@ -3362,9 +3361,6 @@ export function attach(
33623361
// List of owners
33633362
owners,
33643363

3365-
// Location of component in source code.
3366-
source: _debugSource || null,
3367-
33683364
rootType,
33693365
rendererPackageName: renderer.rendererPackageName,
33703366
rendererVersion: renderer.version,
@@ -3725,9 +3721,6 @@ export function attach(
37253721
if (nativeNodes !== null) {
37263722
console.log('Nodes:', nativeNodes);
37273723
}
3728-
if (result.source !== null) {
3729-
console.log('Location:', result.source);
3730-
}
37313724
if (window.chrome || /firefox/i.test(navigator.userAgent)) {
37323725
console.log(
37333726
'Right-click any value to save it as a global variable for further inspection.',

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616

1717
import type {ReactContext, Wakeable} from 'shared/ReactTypes';
18-
import type {Source} from 'shared/ReactElementType';
1918
import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';
2019
import type {
2120
ComponentFilter,
@@ -280,9 +279,6 @@ export type InspectedElement = {
280279
// List of owners
281280
owners: Array<SerializedElement> | null,
282281

283-
// Location of component in source code.
284-
source: Source | null,
285-
286282
type: ElementType,
287283

288284
// Meta information about the root this element belongs to.

packages/react-devtools-shared/src/backendAPI.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,6 @@ export function convertInspectedElementBackendToFrontend(
226226
canViewSource,
227227
hasLegacyContext,
228228
id,
229-
source,
230229
type,
231230
owners,
232231
context,
@@ -261,7 +260,7 @@ export function convertInspectedElementBackendToFrontend(
261260
rendererPackageName,
262261
rendererVersion,
263262
rootType,
264-
source,
263+
source: null, // TODO: Load source location lazily.
265264
type,
266265
owners:
267266
owners === null

packages/react-devtools-shared/src/frontend/types.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
* Be mindful of backwards compatibility when making changes.
1515
*/
1616

17-
import type {Source} from 'shared/ReactElementType';
1817
import type {
1918
Dehydrated,
2019
Unserializable,
@@ -220,7 +219,7 @@ export type InspectedElement = {
220219
owners: Array<SerializedElement> | null,
221220

222221
// Location of component in source code.
223-
source: Source | null,
222+
source: null, // TODO: Reinstate a way to load this lazily.
224223

225224
type: ElementType,
226225

packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ describe('ReactDeprecationWarnings', () => {
131131
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
132132
'Learn more about using refs safely here: ' +
133133
'https://reactjs.org/link/strict-mode-string-ref',
134+
'Warning: Component "Component" contains the string ref "refComponent". ' +
135+
'Support for string refs will be removed in a future major release. We recommend ' +
136+
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
137+
'https://reactjs.org/link/strict-mode-string-ref',
134138
]);
135139
});
136140

@@ -155,14 +159,18 @@ describe('ReactDeprecationWarnings', () => {
155159
}
156160

157161
ReactNoop.render(<Component />);
158-
await expect(async () => await waitForAll([])).toErrorDev(
162+
await expect(async () => await waitForAll([])).toErrorDev([
159163
'Warning: Component "Component" contains the string ref "refComponent". ' +
160164
'Support for string refs will be removed in a future major release. ' +
161165
'This case cannot be automatically converted to an arrow function. ' +
162166
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
163167
'Learn more about using refs safely here: ' +
164168
'https://reactjs.org/link/strict-mode-string-ref',
165-
);
169+
'Warning: Component "Component" contains the string ref "refComponent". ' +
170+
'Support for string refs will be removed in a future major release. We recommend ' +
171+
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
172+
'https://reactjs.org/link/strict-mode-string-ref',
173+
]);
166174
});
167175
}
168176
});

packages/react-dom/src/__tests__/ReactFunctionComponent-test.js

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,6 @@ describe('ReactFunctionComponent', () => {
270270
return <FunctionComponent name="A" ref={() => {}} />;
271271
}
272272
}
273-
Object.defineProperty(AnonymousParentUsingJSX, 'name', {value: undefined});
274273

275274
let instance1;
276275

@@ -293,9 +292,6 @@ describe('ReactFunctionComponent', () => {
293292
});
294293
}
295294
}
296-
Object.defineProperty(AnonymousParentNotUsingJSX, 'name', {
297-
value: undefined,
298-
});
299295

300296
let instance2;
301297
expect(() => {

0 commit comments

Comments
 (0)