Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DevTools: Show hook names based on variable usage #21641

Merged
merged 25 commits into from
Jul 1, 2021
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d75814c
Initial work to support hook variable names in devtools extension.
Jun 24, 2021
3dd1247
Moved named hooks feature before enableHookNameParsing flag
Jun 24, 2021
48632b7
Moved hook names parsing behind user setting
Jun 24, 2021
6a1eecd
Moved feature flag override logic into test helper
Jun 24, 2021
f502611
Added compiled test sources to Git repo
Jun 24, 2021
cb80fba
Test script auto-compiles all source files
Jun 24, 2021
81770cd
Added Jest serializer for Hooks (to sanitize source location)
Jun 25, 2021
d508c43
Removed injectHookVariableNames-test in favor of less hard-coded pars…
Jun 25, 2021
75d2256
Don't generate hook source unless feature flag is enabled
Jun 25, 2021
8075462
Remove hard-coded mappings.wasm file from source
Jun 25, 2021
7f3af1c
Reorganized tests slightly
Jun 25, 2021
49dd494
Fixed stack trace errors. Added additional tests
Jun 26, 2021
f91e8d8
Prevent Jest from configuring Error source-maps
Jun 26, 2021
43bd1b2
Account for Error stack frames having source maps already applied
Jun 28, 2021
475ef1b
Refactored AST utils slightly
Jun 29, 2021
9b7d709
Fixed timeout logic error in hook names cache
Jun 30, 2021
57f15ac
Map hook to hook name (rather than array) for sub hooks
Jun 30, 2021
064da9e
Added DEBUG logging to parseHookNames util
Jun 30, 2021
6222d7f
Updated hook names tests to account for map vs array
Jun 30, 2021
615d8e4
Added option to lazily parse hook names for only the currently inspec…
Jun 30, 2021
9f7a22f
Added an LRU cache and eviction strategy for source code/ASTs
Jun 30, 2021
73fafbe
Enabled external hooks test
Jul 1, 2021
8c4fcb9
Error source map stack frame check is now lazy instead of during modu…
Jul 1, 2021
3bf7311
Styled custom hook names too
Jul 1, 2021
401b94e
Reverted files that were accidentally modified from main branch
Jul 1, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Map hook to hook name (rather than array) for sub hooks
Brian Vaughn committed Jul 1, 2021
commit 57f15ac4e0cab2d6d38fcd3519853ac8bac0cced
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import React, {useEffect, useState} from 'react';

export function Component() {
const [count, setCount] = useState(0);
const isDarkMode = useIsDarkMode();

useEffect(() => {
// ...
}, []);

const handleClick = () => setCount(count + 1);

return (
<>
<div>Dark mode? {isDarkMode}</div>
<div>Count: {count}</div>
<button onClick={handleClick}>Update count</button>
</>
);
}

function useIsDarkMode() {
const [isDarkMode] = useState(false);

useEffect(function useEffectCreate() {
// Here is where we may listen to a "theme" event...
}, []);

return isDarkMode;
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -77,6 +77,10 @@ describe('parseHookNames', () => {
fetch.resetMocks();
});

function expectHookNamesToEqual(map, expectedNamesArray) {
expect(Array.from(map.values())).toEqual(expectedNamesArray);
}

function requireText(path, encoding) {
const {readFileSync} = require('fs');
return readFileSync(path, encoding);
@@ -99,7 +103,7 @@ describe('parseHookNames', () => {
}

const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual(['foo', 'bar', 'baz']);
expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
});

it('should parse names for useReducer()', async () => {
@@ -113,7 +117,7 @@ describe('parseHookNames', () => {
}

const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual(['foo', 'bar', 'baz']);
expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
});

it('should return null for hooks without names like useEffect', async () => {
@@ -126,7 +130,7 @@ describe('parseHookNames', () => {
}

const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual([null, null]);
expectHookNamesToEqual(hookNames, [null, null]);
});

it('should parse names for custom hooks', async () => {
@@ -149,7 +153,7 @@ describe('parseHookNames', () => {
}

const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual(['foo', null, 'baz']);
expectHookNamesToEqual(hookNames, ['foo', null, 'baz']);
});

it('should return null for custom hooks without explicit names', async () => {
@@ -170,15 +174,15 @@ describe('parseHookNames', () => {
}

const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual([null, null]);
expectHookNamesToEqual(hookNames, [null, null]);
});

describe('inline and external source maps', () => {
it('should work for simple components', async () => {
async function test(path) {
const Component = require(path).Component;
const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual([
expectHookNamesToEqual(hookNames, [
'count', // useState
]);
}
@@ -193,7 +197,7 @@ describe('parseHookNames', () => {
const components = require(path);

let hookNames = await getHookNamesForComponent(components.List);
expect(hookNames).toEqual([
expectHookNamesToEqual(hookNames, [
'newItemText', // useState
'items', // useState
'uid', // useState
@@ -207,7 +211,7 @@ describe('parseHookNames', () => {
hookNames = await getHookNamesForComponent(components.ListItem, {
item: {},
});
expect(hookNames).toEqual([
expectHookNamesToEqual(hookNames, [
'handleDelete', // useCallback
'handleToggle', // useCallback
]);
@@ -218,12 +222,30 @@ describe('parseHookNames', () => {
await test('./__source__/__compiled__/external/ToDoList'); // external source map
});

it('should work for custom hook', async () => {
async function test(path) {
const Component = require(path).Component;
const hookNames = await getHookNamesForComponent(Component);
expectHookNamesToEqual(hookNames, [
'count', // useState()
'isDarkMode', // useIsDarkMode()
'isDarkMode', // useIsDarkMode -> useState()
null,
null,
]);
}

await test('./__source__/ComponentWithCustomHook'); // original source (uncompiled)
await test('./__source__/__compiled__/inline/ComponentWithCustomHook'); // inline source map
await test('./__source__/__compiled__/external/ComponentWithCustomHook'); // external source map
});

// TODO (named hooks) The useContext() line number is slightly off
xit('should work for external hooks', async () => {
async function test(path) {
const Component = require(path).Component;
const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual([
expectHookNamesToEqual(hookNames, [
'theme', // useTheme()
'theme', // useContext()
]);
@@ -239,11 +261,13 @@ describe('parseHookNames', () => {
});

// TODO (named hooks) Inline require (e.g. require("react").useState()) isn't supported yet
// Maybe this isn't an important use case to support,
// since inline requires are most likely to exist in compiled source (if at all).
xit('should work for inline requires', async () => {
async function test(path) {
const Component = require(path).Component;
const hookNames = await getHookNamesForComponent(Component);
expect(hookNames).toEqual([
expectHookNamesToEqual(hookNames, [
'count', // useState()
]);
}
100 changes: 54 additions & 46 deletions packages/react-devtools-extensions/src/parseHookNames.js
Original file line number Diff line number Diff line change
@@ -20,7 +20,7 @@ import type {
HookSource,
HooksTree,
} from 'react-debug-tools/src/ReactDebugHooks';
import type {HookNames} from 'react-devtools-shared/src/hookNamesCache';
import type {HookNames} from 'react-devtools-shared/src/types';
import type {Thenable} from 'shared/ReactTypes';
import type {SourceConsumer, SourceMap} from './astUtils';

@@ -95,6 +95,10 @@ export default async function parseHookNames(

// TODO (named hooks) Call .destroy() on SourceConsumers after we're done to free up memory.

// TODO (named hooks) Replace Array of hook names with a Map() of hook ID to name to better support mapping nested hook to name.
// This is a little tricky though, since custom hooks don't have IDs.
// We may need to add an ID for these too, in the backend?

return loadSourceFiles(fileNameToHookSourceData)
.then(() => extractAndLoadSourceMaps(fileNameToHookSourceData))
.then(() => parseSourceAST(fileNameToHookSourceData))
@@ -203,58 +207,62 @@ function fetchFile(url: string): Promise<string> {
function findHookNames(
hooksList: Array<HooksNode>,
fileNameToHookSourceData: Map<string, HookSourceData>,
): Promise<HookNames> {
return ((Promise.all(
hooksList.map(hook => {
if (isNonDeclarativePrimitiveHook(hook)) {
// Not all hooks have names (e.g. useEffect or useLayoutEffect)
return null;
}
): HookNames {
const map: HookNames = new Map();

// We already guard against a null HookSource in parseHookNames()
const hookSource = ((hook.hookSource: any): HookSource);
const fileName = hookSource.fileName;
if (!fileName) {
return null; // Should not be reachable.
}
hooksList.map(hook => {
if (isNonDeclarativePrimitiveHook(hook)) {
// Not all hooks have names (e.g. useEffect or useLayoutEffect)
return null;
}

const hookSourceData = fileNameToHookSourceData.get(fileName);
if (!hookSourceData) {
return null; // Should not be reachable.
}
// We already guard against a null HookSource in parseHookNames()
const hookSource = ((hook.hookSource: any): HookSource);
const fileName = hookSource.fileName;
if (!fileName) {
return null; // Should not be reachable.
}

const {lineNumber, columnNumber} = hookSource;
if (!lineNumber || !columnNumber) {
return null; // Should not be reachable.
}
const hookSourceData = fileNameToHookSourceData.get(fileName);
if (!hookSourceData) {
return null; // Should not be reachable.
}

const sourceConsumer = hookSourceData.sourceConsumer;
const {lineNumber, columnNumber} = hookSource;
if (!lineNumber || !columnNumber) {
return null; // Should not be reachable.
}

let originalSourceLineNumber;
if (sourceMapsAreAppliedToErrors || !sourceConsumer) {
// Either the current environment automatically applies source maps to errors,
// or the current code had no source map to begin with.
// Either way, we don't need to convert the Error stack frame locations.
originalSourceLineNumber = lineNumber;
} else {
originalSourceLineNumber = sourceConsumer.originalPositionFor({
line: lineNumber,
column: columnNumber,
}).line;
}
const sourceConsumer = hookSourceData.sourceConsumer;

if (originalSourceLineNumber === null) {
return null;
}
let originalSourceLineNumber;
if (sourceMapsAreAppliedToErrors || !sourceConsumer) {
// Either the current environment automatically applies source maps to errors,
// or the current code had no source map to begin with.
// Either way, we don't need to convert the Error stack frame locations.
originalSourceLineNumber = lineNumber;
} else {
originalSourceLineNumber = sourceConsumer.originalPositionFor({
line: lineNumber,
column: columnNumber,
}).line;
}

return getHookName(
hook,
hookSourceData.originalSourceAST,
((hookSourceData.originalSourceCode: any): string),
((originalSourceLineNumber: any): number),
);
}),
): any): Promise<HookNames>);
if (originalSourceLineNumber === null) {
return null;
}

const name = getHookName(
hook,
hookSourceData.originalSourceAST,
((hookSourceData.originalSourceCode: any): string),
((originalSourceLineNumber: any): number),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we assuming that a line number is sufficient to identify a hook call? Seems like that assumption breaks down in edge cases:

  1. Minified code (either without a source map, or a bundle built from pre-minified inputs) can have multiple hooks on one line.
  2. Some code might be authored with quirky formatting, i.e. not with a typical Prettier / ESLint setup.

IMO it's safer to use all the information we have from the source map / AST and consider both lines and columns everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's safer to use all the information we have from the source map / AST and consider both lines and columns everywhere.

Yeah. Sounds reasonable. I've created #21792 to follow up.

);

map.set(hook, name);
});

return map;
}

function isValidUrl(possibleURL: string): boolean {
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@
* It should always be imported from "react-devtools-feature-flags".
************************************************************************/

export const enableHookNameParsing = false;
export const enableHookNameParsing = true;
export const enableProfilerChangedHookIndices = false;
export const isInternalFacebookBuild = false;

Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ import {ElementTypeFunction} from 'react-devtools-shared/src/types';
import LoadHookNamesFunctionContext from 'react-devtools-shared/src/devtools/views/Components/LoadHookNamesFunctionContext';
import {SettingsContext} from '../Settings/SettingsContext';

import type {HookNames} from 'react-devtools-shared/src/hookNamesCache';
import type {HookNames} from 'react-devtools-shared/src/types';
import type {ReactNodeList} from 'shared/ReactTypes';
import type {
Element,
Original file line number Diff line number Diff line change
@@ -25,12 +25,13 @@ import {enableProfilerChangedHookIndices} from 'react-devtools-feature-flags';
import type {InspectedElement} from './types';
import type {HooksNode, HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
import type {HookNames} from 'react-devtools-shared/src/types';
import type {Element} from 'react-devtools-shared/src/devtools/views/Components/types';

type HooksTreeViewProps = {|
bridge: FrontendBridge,
element: Element,
hookNames: Array<string> | null,
hookNames: HookNames | null,
inspectedElement: InspectedElement,
store: Store,
|};
@@ -72,7 +73,7 @@ export function InspectedElementHooksTree({

type InnerHooksTreeViewProps = {|
element: Element,
hookNames: Array<string> | null,
hookNames: HookNames | null,
hooks: HooksTree,
id: number,
inspectedElement: InspectedElement,
@@ -93,7 +94,7 @@ export function InnerHooksTreeView({
key={index}
element={element}
hook={hooks[index]}
hookName={hookNames != null ? hookNames[index] : null}
hookNames={hookNames}
id={id}
inspectedElement={inspectedElement}
path={path.concat([index])}
@@ -104,7 +105,7 @@ export function InnerHooksTreeView({
type HookViewProps = {|
element: Element,
hook: HooksNode,
hookName: string | null,
hookNames: HookNames | null,
id: number,
inspectedElement: InspectedElement,
path: Array<string | number>,
@@ -113,7 +114,7 @@ type HookViewProps = {|
function HookView({
element,
hook,
hookName,
hookNames,
id,
inspectedElement,
path,
@@ -193,6 +194,8 @@ function HookView({

let displayValue;
let isComplexDisplayValue = false;

const hookName = hookNames != null ? hookNames.get(hook) : null;
const hookDisplayName = hookName ? `${name}(${hookName})` : name;

// Format data for display to mimic the props/state/context for now.
@@ -219,6 +222,7 @@ function HookView({
<InnerHooksTreeView
element={element}
hooks={subHooks}
hookNames={hookNames}
id={id}
inspectedElement={inspectedElement}
path={path.concat(['subHooks'])}
Original file line number Diff line number Diff line change
@@ -36,14 +36,14 @@ import styles from './InspectedElementView.css';

import type {ContextMenuContextType} from '../context';
import type {Element, InspectedElement, SerializedElement} from './types';
import type {ElementType} from 'react-devtools-shared/src/types';
import type {ElementType, HookNames} from 'react-devtools-shared/src/types';

export type CopyPath = (path: Array<string | number>) => void;
export type InspectPath = (path: Array<string | number>) => void;

type Props = {|
element: Element,
hookNames: Array<string> | null,
hookNames: HookNames | null,
inspectedElement: InspectedElement,
|};

Original file line number Diff line number Diff line change
@@ -38,7 +38,7 @@ import './root.css';
import type {HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
import type {InspectedElement} from 'react-devtools-shared/src/devtools/views/Components/types';
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
import type {HookNames} from 'react-devtools-shared/src/hookNamesCache';
import type {HookNames} from 'react-devtools-shared/src/types';
import type {Thenable} from '../cache';

export type BrowserTheme = 'dark' | 'light';
9 changes: 5 additions & 4 deletions packages/react-devtools-shared/src/hookNamesCache.js
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ import {enableHookNameParsing} from 'react-devtools-feature-flags';
import type {HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
import type {Thenable, Wakeable} from 'shared/ReactTypes';
import type {Element} from './devtools/views/Components/types';
import type {HookNames} from 'react-devtools-shared/src/types';

const TIMEOUT = 3000;

@@ -49,9 +50,6 @@ function readRecord<T>(record: Record<T>): ResolvedRecord<T> | RejectedRecord {
}
}

export type HookName = string | null;
export type HookNames = Array<HookName>;

type HookNamesMap = WeakMap<Element, Record<HookNames>>;

function createMap(): HookNamesMap {
@@ -75,7 +73,10 @@ export function loadHookNames(

// TODO (named hooks) Make sure caching layer works and we aren't re-parsing ASTs.
let record = map.get(element);
if (!record) {
if (record) {
// TODO (named hooks) Do we need to update the Map to use new hooks list as keys?
// or wil these be stable between inspections as a component updates?
} else {
const callbacks = new Set();
const wakeable: Wakeable = {
then(callback) {
5 changes: 5 additions & 0 deletions packages/react-devtools-shared/src/types.js
Original file line number Diff line number Diff line change
@@ -7,6 +7,8 @@
* @flow
*/

import type {HooksNode} from 'react-debug-tools/src/ReactDebugHooks';

export type Wall = {|
// `listen` returns the "unlisten" function.
listen: (fn: Function) => Function,
@@ -76,3 +78,6 @@ export type ComponentFilter =
| BooleanComponentFilter
| ElementTypeComponentFilter
| RegExpComponentFilter;

export type HookName = string | null;
export type HookNames = Map<HooksNode, HookName>;