Skip to content

Commit 79f6f34

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

File tree

8 files changed

+245
-75
lines changed

8 files changed

+245
-75
lines changed

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

+1-55
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

+16
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

+18
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

+112
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
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 ErrorStackParser from 'error-stack-parser';
11+
import {isInternalModule} from '../moduleFilters';
12+
13+
import * as moduleOne from './__modules__/module-one';
14+
import * as moduleTwo from './__modules__/module-two';
15+
16+
describe('isInternalModule', () => {
17+
let moduleSourceToRanges;
18+
19+
function parseAndGetStackFrame(error, index = 0) {
20+
const stackFrame = ErrorStackParser.parse(error)[index];
21+
22+
// Strip out the portion of the file path that includes "react-devtools-scheduling-profiler"
23+
// or every frame will be flagged as internal.
24+
stackFrame.fileName = stackFrame.fileName.replace(
25+
/.+__tests__/,
26+
'../__tests__',
27+
);
28+
29+
return stackFrame;
30+
}
31+
32+
function stackFrameToFlamechartStackFrame(stackFrame) {
33+
return {
34+
name: 'test',
35+
timestamp: 0,
36+
duration: 1,
37+
scriptUrl: stackFrame.fileName,
38+
locationLine: stackFrame.lineNumber,
39+
locationColumn: stackFrame.columnNumber,
40+
};
41+
}
42+
43+
beforeEach(() => {
44+
moduleSourceToRanges = new Map();
45+
46+
let startErrorStackFrame = parseAndGetStackFrame(
47+
moduleOne.moduleStartError,
48+
);
49+
let stopErrorStackFrame = parseAndGetStackFrame(moduleOne.moduleStopError);
50+
moduleSourceToRanges.set(startErrorStackFrame.fileName, [
51+
[startErrorStackFrame, stopErrorStackFrame],
52+
]);
53+
54+
const moduleTwoRanges = [];
55+
56+
startErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleAStartError);
57+
stopErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleAStopError);
58+
moduleTwoRanges.push([startErrorStackFrame, stopErrorStackFrame]);
59+
60+
startErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleBStartError);
61+
stopErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleBStopError);
62+
moduleTwoRanges.push([startErrorStackFrame, stopErrorStackFrame]);
63+
64+
moduleSourceToRanges.set(startErrorStackFrame.fileName, moduleTwoRanges);
65+
});
66+
67+
it('should properly identify stack frames within the provided module ranges', () => {
68+
let flamechartStackFrame = stackFrameToFlamechartStackFrame(
69+
parseAndGetStackFrame(moduleOne.innerError),
70+
);
71+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
72+
true,
73+
);
74+
75+
flamechartStackFrame = stackFrameToFlamechartStackFrame(
76+
parseAndGetStackFrame(moduleTwo.innerErrorA),
77+
);
78+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
79+
true,
80+
);
81+
82+
flamechartStackFrame = stackFrameToFlamechartStackFrame(
83+
parseAndGetStackFrame(moduleTwo.innerErrorB),
84+
);
85+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
86+
true,
87+
);
88+
});
89+
90+
it('should properly identify stack frames outside of the provided module ranges', () => {
91+
let flamechartStackFrame = stackFrameToFlamechartStackFrame(
92+
parseAndGetStackFrame(moduleOne.outerErrorA),
93+
);
94+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
95+
false,
96+
);
97+
98+
flamechartStackFrame = stackFrameToFlamechartStackFrame(
99+
parseAndGetStackFrame(moduleOne.outerErrorB),
100+
);
101+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
102+
false,
103+
);
104+
105+
flamechartStackFrame = stackFrameToFlamechartStackFrame(
106+
parseAndGetStackFrame(moduleTwo.outerError),
107+
);
108+
expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe(
109+
false,
110+
);
111+
});
112+
});

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

+69
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

+1-1
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

+25-9
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

+3-10
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)