Skip to content

Commit c3439e9

Browse files
author
Brian Vaughn
committed
Refactored code so that isInternalModule function could be unit tested.
1 parent a6f6461 commit c3439e9

File tree

8 files changed

+212
-75
lines changed

8 files changed

+212
-75
lines changed

‎packages/react-devtools-scheduling-profiler/src/content-views/FlamechartView.js

Lines changed: 1 addition & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,6 @@ import type {
2121
ViewRefs,
2222
} from '../view-base';
2323

24-
import {
25-
CHROME_WEBSTORE_EXTENSION_ID,
26-
INTERNAL_EXTENSION_ID,
27-
LOCAL_EXTENSION_ID,
28-
} from 'react-devtools-shared/src/constants';
2924
import {
3025
BackgroundColorView,
3126
Surface,
@@ -36,6 +31,7 @@ import {
3631
rectIntersectsRect,
3732
verticallyStackedLayout,
3833
} from '../view-base';
34+
import {isInternalModule} from './utils/moduleFilters';
3935
import {
4036
durationToWidth,
4137
positioningScaleFactor,
@@ -75,56 +71,6 @@ function hoverColorForStackFrame(stackFrame: FlamechartStackFrame): string {
7571
return hslaColorToString(color);
7672
}
7773

78-
function isInternalModule(
79-
internalModuleSourceToRanges: InternalModuleSourceToRanges,
80-
flamechartStackFrame: FlamechartStackFrame,
81-
): boolean {
82-
const {locationColumn, locationLine, scriptUrl} = flamechartStackFrame;
83-
84-
if (scriptUrl == null || locationColumn == null || locationLine == null) {
85-
return true;
86-
}
87-
88-
// Internal modules are only registered if DevTools was running when the profile was captured,
89-
// but DevTools should also hide its own frames to avoid over-emphasizing them.
90-
if (
91-
// Handle webpack-internal:// sources
92-
scriptUrl.includes('/react-devtools') ||
93-
scriptUrl.includes('/react_devtools') ||
94-
// Filter out known extension IDs
95-
scriptUrl.includes(CHROME_WEBSTORE_EXTENSION_ID) ||
96-
scriptUrl.includes(INTERNAL_EXTENSION_ID) ||
97-
scriptUrl.includes(LOCAL_EXTENSION_ID)
98-
99-
// Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files.
100-
) {
101-
return true;
102-
}
103-
104-
// Filter out React internal packages.
105-
const ranges = internalModuleSourceToRanges.get(scriptUrl);
106-
if (ranges != null) {
107-
for (let i = 0; i < ranges.length; i++) {
108-
const [startStackFrame, stopStackFrame] = ranges[i];
109-
110-
const isAfterStart =
111-
locationLine > startStackFrame.lineNumber ||
112-
(locationLine === startStackFrame.lineNumber &&
113-
locationColumn >= startStackFrame.columnNumber);
114-
const isBeforeStop =
115-
locationLine < stopStackFrame.lineNumber ||
116-
(locationLine === stopStackFrame.lineNumber &&
117-
locationColumn <= stopStackFrame.columnNumber);
118-
119-
if (isAfterStart && isBeforeStop) {
120-
return true;
121-
}
122-
}
123-
}
124-
125-
return false;
126-
}
127-
12874
class FlamechartStackLayerView extends View {
12975
/** Layer to display */
13076
_stackLayer: FlamechartStackLayer;

‎packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-one.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow
8+
*/
9+
10+
export const outerErrorA = new Error();
11+
12+
export const moduleStartError = new Error();
13+
export const innerError = new Error();
14+
export const moduleStopError = new Error();
15+
16+
export const outerErrorB = new Error();

‎packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-two.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow
8+
*/
9+
10+
export const moduleAStartError = new Error();
11+
export const innerErrorA = new Error();
12+
export const moduleAStopError = new Error();
13+
14+
export const outerError = new Error();
15+
16+
export const moduleBStartError = new Error();
17+
export const innerErrorB = new Error();
18+
export const moduleBStopError = new Error();

‎packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/moduleFilters-test.js

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow
8+
*/
9+
10+
import {isInternalModule} from '../moduleFilters';
11+
12+
describe('isInternalModule', () => {
13+
let map;
14+
15+
function createFlamechartStackFrame(scriptUrl, locationLine, locationColumn) {
16+
return {
17+
name: 'test',
18+
timestamp: 0,
19+
duration: 1,
20+
scriptUrl,
21+
locationLine,
22+
locationColumn,
23+
};
24+
}
25+
26+
function createStackFrame(fileName, lineNumber, columnNumber) {
27+
return {
28+
columnNumber: columnNumber,
29+
lineNumber: lineNumber,
30+
fileName: fileName,
31+
functionName: 'test',
32+
source: ` at test (${fileName}:${lineNumber}:${columnNumber})`,
33+
};
34+
}
35+
36+
beforeEach(() => {
37+
map = new Map();
38+
map.set('foo', [
39+
[createStackFrame('foo', 10, 0), createStackFrame('foo', 15, 100)],
40+
]);
41+
map.set('bar', [
42+
[createStackFrame('bar', 10, 0), createStackFrame('bar', 15, 100)],
43+
[createStackFrame('bar', 20, 0), createStackFrame('bar', 25, 100)],
44+
]);
45+
});
46+
47+
it('should properly identify stack frames within the provided module ranges', () => {
48+
expect(
49+
isInternalModule(map, createFlamechartStackFrame('foo', 10, 0)),
50+
).toBe(true);
51+
expect(
52+
isInternalModule(map, createFlamechartStackFrame('foo', 12, 35)),
53+
).toBe(true);
54+
expect(
55+
isInternalModule(map, createFlamechartStackFrame('foo', 15, 100)),
56+
).toBe(true);
57+
expect(
58+
isInternalModule(map, createFlamechartStackFrame('bar', 12, 0)),
59+
).toBe(true);
60+
expect(
61+
isInternalModule(map, createFlamechartStackFrame('bar', 22, 125)),
62+
).toBe(true);
63+
});
64+
65+
it('should properly identify stack frames outside of the provided module ranges', () => {
66+
expect(isInternalModule(map, createFlamechartStackFrame('foo', 9, 0))).toBe(
67+
false,
68+
);
69+
expect(
70+
isInternalModule(map, createFlamechartStackFrame('foo', 15, 101)),
71+
).toBe(false);
72+
expect(
73+
isInternalModule(map, createFlamechartStackFrame('bar', 17, 0)),
74+
).toBe(false);
75+
expect(
76+
isInternalModule(map, createFlamechartStackFrame('baz', 12, 0)),
77+
).toBe(false);
78+
});
79+
});

‎packages/react-devtools-scheduling-profiler/src/content-views/utils/moduleFilters.js

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow
8+
*/
9+
10+
import type {
11+
FlamechartStackFrame,
12+
InternalModuleSourceToRanges,
13+
} from '../../types';
14+
15+
import {
16+
CHROME_WEBSTORE_EXTENSION_ID,
17+
INTERNAL_EXTENSION_ID,
18+
LOCAL_EXTENSION_ID,
19+
} from 'react-devtools-shared/src/constants';
20+
21+
export function isInternalModule(
22+
internalModuleSourceToRanges: InternalModuleSourceToRanges,
23+
flamechartStackFrame: FlamechartStackFrame,
24+
): boolean {
25+
const {locationColumn, locationLine, scriptUrl} = flamechartStackFrame;
26+
27+
if (scriptUrl == null || locationColumn == null || locationLine == null) {
28+
// This could indicate a browser-internal API like performance.mark().
29+
return false;
30+
}
31+
32+
// Internal modules are only registered if DevTools was running when the profile was captured,
33+
// but DevTools should also hide its own frames to avoid over-emphasizing them.
34+
if (
35+
// Handle webpack-internal:// sources
36+
scriptUrl.includes('/react-devtools') ||
37+
scriptUrl.includes('/react_devtools') ||
38+
// Filter out known extension IDs
39+
scriptUrl.includes(CHROME_WEBSTORE_EXTENSION_ID) ||
40+
scriptUrl.includes(INTERNAL_EXTENSION_ID) ||
41+
scriptUrl.includes(LOCAL_EXTENSION_ID)
42+
// Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files.
43+
) {
44+
return true;
45+
}
46+
47+
// Filter out React internal packages.
48+
const ranges = internalModuleSourceToRanges.get(scriptUrl);
49+
if (ranges != null) {
50+
for (let i = 0; i < ranges.length; i++) {
51+
const [startStackFrame, stopStackFrame] = ranges[i];
52+
53+
const isAfterStart =
54+
locationLine > startStackFrame.lineNumber ||
55+
(locationLine === startStackFrame.lineNumber &&
56+
locationColumn >= startStackFrame.columnNumber);
57+
const isBeforeStop =
58+
locationLine < stopStackFrame.lineNumber ||
59+
(locationLine === stopStackFrame.lineNumber &&
60+
locationColumn <= stopStackFrame.columnNumber);
61+
62+
if (isAfterStart && isBeforeStop) {
63+
return true;
64+
}
65+
}
66+
}
67+
68+
return false;
69+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ export type DevToolsHook = {
421421
) => void,
422422

423423
// Scheduling Profiler internal module filtering
424-
getInternalModuleRanges: () => Array<[Error, Error]>,
424+
getInternalModuleRanges: () => Array<[string, string]>,
425425
registerInternalModuleStart: (moduleStartError: Error) => void,
426426
registerInternalModuleStop: (moduleStopError: Error) => void,
427427

‎packages/react-devtools-shared/src/hook.js

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -490,21 +490,37 @@ export function installHook(target: any): DevToolsHook | null {
490490
}
491491
}
492492

493-
const openModuleRangesStack: Array<Error> = [];
494-
const moduleRanges: Array<[Error, Error]> = [];
493+
type StackFrameString = string;
495494

496-
function getInternalModuleRanges(): Array<[Error, Error]> {
495+
const openModuleRangesStack: Array<StackFrameString> = [];
496+
const moduleRanges: Array<[StackFrameString, StackFrameString]> = [];
497+
498+
function getTopStackFrameString(error: Error): StackFrameString | null {
499+
const frames = error.stack.split('\n');
500+
const frame = frames.length > 1 ? frames[1] : null;
501+
return frame;
502+
}
503+
504+
function getInternalModuleRanges(): Array<
505+
[StackFrameString, StackFrameString],
506+
> {
497507
return moduleRanges;
498508
}
499509

500-
function registerInternalModuleStart(moduleStartError: Error) {
501-
openModuleRangesStack.push(moduleStartError);
510+
function registerInternalModuleStart(error: Error) {
511+
const startStackFrame = getTopStackFrameString(error);
512+
if (startStackFrame !== null) {
513+
openModuleRangesStack.push(startStackFrame);
514+
}
502515
}
503516

504-
function registerInternalModuleStop(moduleStopError: Error) {
505-
const moduleStartError = openModuleRangesStack.pop();
506-
if (moduleStartError) {
507-
moduleRanges.push([moduleStartError, moduleStopError]);
517+
function registerInternalModuleStop(error: Error) {
518+
if (openModuleRangesStack.length > 0) {
519+
const startStackFrame = openModuleRangesStack.pop();
520+
const stopStackFrame = getTopStackFrameString(error);
521+
if (stopStackFrame !== null) {
522+
moduleRanges.push([startStackFrame, stopStackFrame]);
523+
}
508524
}
509525
}
510526

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -106,17 +106,10 @@ function markInternalModuleRanges() {
106106
) {
107107
const ranges = __REACT_DEVTOOLS_GLOBAL_HOOK__.getInternalModuleRanges();
108108
for (let i = 0; i < ranges.length; i++) {
109-
const [startError, stopError] = ranges[i];
109+
const [startStackFrame, stopStackFrame] = ranges[i];
110110

111-
// Don't embed Error stack parsing logic into the reconciler.
112-
// Just serialize the top stack frame and let the profiler parse it.
113-
const startFrames = startError.stack.split('\n');
114-
const startFrame = startFrames.length > 1 ? startFrames[1] : '';
115-
const stopFrames = stopError.stack.split('\n');
116-
const stopFrame = stopFrames.length > 1 ? stopFrames[1] : '';
117-
118-
markAndClear(`--react-internal-module-start-${startFrame}`);
119-
markAndClear(`--react-internal-module-stop-${stopFrame}`);
111+
markAndClear(`--react-internal-module-start-${startStackFrame}`);
112+
markAndClear(`--react-internal-module-stop-${stopStackFrame}`);
120113
}
121114
}
122115
}

0 commit comments

Comments
 (0)