Supporting hook variable names in devtools extension #115

vibhorgupta-gh · 2020-11-21T08:29:02Z

Solves #72 #80 #116 #117 #118 #119

To obtain the names of the hooks, we adopt the following approach -

Inject a function in the Devtools Backend (injectHookVariableNamesFunction)
Obtain the information of hooks by modifying the stackTrace in the Devtools Backend
Load the source maps and source code for the hook under observation
Parse the AST of the source code and isolate the AST node for the hook under observation
Extract the variable name for the hook from its AST node

bvaughn

Just did a quick read through and left a few thoughts. Exciting progress!

packages/react-devtools-extensions/src/main.js

packages/react-devtools-extensions/src/utils.js

packages/react-devtools-shared/package.json

packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js

packages/react-devtools-extensions/src/main.js

1. Pass hookVariableName as a param with Hooks 2. Ignore non declarative primitive hooks

Changed WASM to refer to local copy of mappings.wasm Warn only for Dev mode

vibhorgupta-gh · 2020-12-17T23:26:55Z

Hi @bvaughn, some updates:

Fixed existing tests in inspectedElementContext-test.js
Wrote unit tests for all hook declaration cases we could think of (sans tests which check for names being injected in sub hooks, still figuring out the best way to do those)
Wrote snapshot tests for all cases we could think of (again, sans the ones involving injecting names in sub hooks)

We had some questions:

Regarding end to end testing, The way we are doing it right now is basically defining a hook log (we picked the one we used to test in our sample app) and setting the appropriate source file name. We pick and drop the bundle file and the source map file from our systems into the resources folder. We then mock the fetch function and call injectHookVariableNamesFunction with this hook log. Not sure if this is the best method though, we'd ideally like there to be some sort of generator script that runs in a beforeAll block that just takes a sample React file in resources and uses webpack to generate the bundle and source map on runtime which we can utilize (instead of hardcoding these resources). Our progress in case of end to end tests is still ongoing in commit 3c08354. Commits prior to these (unit/snapshot tests) are mostly done. If you could review those and try breaking these test suites for some cases that we might have missed, that'd be helpful next steps.
The tests in packages/react-debug-tools/src/__tests__/ are currently failing as well, but we know what causes the fail, the hook tests in the file use an outdated structure of each hook node in hook log, updating which should fix everything. Shall we go ahead with the fix there?

mknmohit · 2021-05-05T14:10:51Z

Excited to see this useful feature in future. Much appreciated your efforts 👍

ankursnh023 · 2021-05-19T06:52:17Z

Any updates when this feature will be released?
@bvaughn @VibhorCodecianGupta

bvaughn · 2021-06-07T19:16:12Z

I'm going to try to rebase this branch now. Looks like it's got a few conflicts. Hopefully it won't be too crazy 😅

The logic around suspending to inspect the selected element has changed a lot in recent months, to improve the Suspense/cache integration, so there are non-trivial conflicts in InspectedElementContext.

bvaughn · 2021-06-07T20:26:17Z

Regarding end to end testing, The way we are doing it right now is basically defining a hook log (we picked the one we used to test in our sample app) and setting the appropriate source file name. We pick and drop the bundle file and the source map file from our systems into the resources folder. We then mock the fetch function and call injectHookVariableNamesFunction with this hook log. Not sure if this is the best method though, we'd ideally like there to be some sort of generator script that runs in a beforeAll block that just takes a sample React file in resources and uses webpack to generate the bundle and source map on runtime which we can utilize (instead of hardcoding these resources).

@VibhorCodecianGupta Agreed that this would be nicer than embedding the bundle.js file. That static file is a little confusing (how do I update it? Where is the source of truth?) and will likely cause problems for our Prettier and Lint tooling unless we ignore it, but really ideally we'd just generate it lazily as part of testing 😄 Assuming this wasn't super slow?

bvaughn · 2021-06-07T20:52:12Z

I've merged master into this branch and resolved a lot of conflicts, but some of them are pretty large (like in the inspected element context) so I'm going to be working on merging/rewriting that section for a little bit. For now, I'm going to be pushing my changes to a separate branch: facebook#21641

If either @saphal1998 or @VibhorCodecianGupta would be interested in talking through some of these changes, I'd be happy to. I know it's been a while though and the MLH program is over, so if you're both too busy– I understand that as well!

Thanks for the work you did putting this PR together. :)

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

bvaughn · 2021-06-08T23:20:50Z

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

+function getSourceMapURL(url: string, urlResponse: string): string {
+  const sourceMappingUrlRegExp = /\/\/[#@] ?sourceMappingURL=([^\s'"]+)\s*$/mg
+  const sourceMappingURLInArray = urlResponse.match(sourceMappingUrlRegExp);
+  if (sourceMappingURLInArray && sourceMappingURLInArray.length > 1) {


Code Sandbox uses base64 encoded source maps. So maybe we could have a branch here that loads the file or decodes the inline source map using atob()

saphal1998 · 2021-06-11T17:05:29Z

Hi @bvaughn! We would love to help out in any way we can!

Unfortunately though, both @VibhorCodecianGupta and I, have lost context of the nitty gritty of the work done, we'll just go over it this PR again this weekend, to get some context back so that we are in better position to give inputs.

Can you let us know if you have any specific things you want us to look into so that we can have a more focussed discussion on this?

bvaughn · 2021-06-11T17:08:33Z

Hi @saphal1998!

I've opened a PR against the React repo over here: facebook#21641

I've jotted down a checklist of things to do or look at there. I'll just be chipping away at them. If there's anything you or @VibhorCodecianGupta would be interested in picking up, let's just chat and you could maybe open a PR to my branch 😄

bvaughn · 2021-06-17T21:05:41Z

Hi again @saphal1998 and @VibhorCodecianGupta 😁

I've added some additional unit tests here:
https://github.com/bvaughn/react/blob/devtools-named-hooks/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js

Which includes some tests that use inline and external source maps (using this test component compiled by this script).

Unfortunately the test fails b'c there's a line number mismatch between the compiled source and source-map parsed original source, which causes the useState hook inside of useCustomHook to fail:

function useCustomHook() {
  const [stateValue] = useState(true);

  return stateValue;
}

~~I'm a little stumped and would welcome any ideas you two have 😄 ~~

Edit It seems that the majority of this was due to a Jest runner opting Errors into automatically taking source maps into accountability. This makes sense for testing simplicity in most cases, but was horribly breaking the logic in these tests specifically, so this behavior was disabled in facebook@9f4627f.

mknmohit · 2021-06-25T16:57:07Z

packages/react-debug-tools/src/ReactDebugHooks.js

@@ -516,13 +529,22 @@ function buildTree(rootStack, readHookLog): HooksTree {

    // For the time being, only State and Reducer hooks support runtime overrides.
    const isStateEditable = primitive === 'Reducer' || primitive === 'State';
+    const hookSource: HookSource = {lineNumber: null, functionName: null, fileName: null, columnNumber: null}
+    if (stack && stack.length >= 1) {


OR

Suggested change

if (stack && stack.length >= 1) {

if (stack?.length) {

mknmohit · 2021-06-25T17:06:01Z

packages/react-devtools-shared/src/devtools/views/Components/InspectedElementHooksTree.js

@@ -172,6 +172,7 @@ function HookView({

  let displayValue;
  let isComplexDisplayValue = false;
+  const customHookDisplayName = hookVariableName ? `${name}(${hookVariableName})` : name;


mknmohit · 2021-06-25T17:12:49Z

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

+  value: mixed,
+  subHooks: Array<HooksNode>,
+  hookSource: HookSource,
+  ...


I am just curious to know why you have spread operator here, I mean what's it's doing here? 🤔

bvaughn · 2021-07-01T18:39:25Z

This feature has just been merged into the React main branch via facebook#21641

Thank you for all of your hard work! 🥳

cmacdonnacha · 2021-07-01T18:46:46Z

Excellent work @bvaughn . How do we go about getting the latest update? Is it just a matter of updating the chrome extension?

bvaughn · 2021-07-01T18:51:00Z

It will probably be released with the next major DevTools version, once it's been tested inside of Facebook for a bit. This is a pretty big feature and I want to give it a little of time to smoke test with a few hundred engineers before pushing it out to millions.

saphal1998 · 2021-07-01T19:08:48Z

@bvaughn thanks a lot for picking this up! We have not been able to give this any time at all due to other prior commitments, so we really appreciate this!

vibhorgupta-gh changed the title ~~Injecting hook variable names in devtools~~ [WIP]: Injecting hook variable names in devtools Nov 21, 2020

vibhorgupta-gh changed the title ~~[WIP]: Injecting hook variable names in devtools~~ [WIP]: Supporting hook variable names in devtools extension Nov 21, 2020

vibhorgupta-gh force-pushed the lineNumberOfHooks branch 2 times, most recently from 7401fb3 to dd932fe Compare November 30, 2020 18:04

saphal1998 and others added 4 commits November 30, 2020 23:42

Added injectHookVariableNamesFunction into DevTools

09a588c

Filled in some Suspense/cache scaffolding

11f9d6e

Added scaffolding for utilizing source maps

8ec51d6

Generating and parsing ASTs to obtain hook variable names

03aaaef

vibhorgupta-gh force-pushed the lineNumberOfHooks branch from dd932fe to c6ba425 Compare November 30, 2020 18:38

vibhorgupta-gh changed the title ~~[WIP]: Supporting hook variable names in devtools extension~~ Supporting hook variable names in devtools extension Nov 30, 2020

saphal1998 added this to the Fellowship - Fall 2020 milestone Nov 30, 2020

saphal1998 assigned saphal1998 and vibhorgupta-gh Nov 30, 2020

bvaughn reviewed Nov 30, 2020

View reviewed changes

vibhorgupta-gh and others added 2 commits December 1, 2020 02:39

Adding support for custom hooks and sub hooks

0dc4d1b

Bug fixes

1bd801d

1. Pass hookVariableName as a param with Hooks 2. Ignore non declarative primitive hooks

vibhorgupta-gh force-pushed the lineNumberOfHooks branch from c6ba425 to 1bd801d Compare November 30, 2020 21:11

vibhorgupta-gh and others added 3 commits December 1, 2020 20:41

nits: removed comments and stray console logs

f7206a2

fix: frontend render for custom hook variable names

9873685

Nits:

5fae37c

Changed WASM to refer to local copy of mappings.wasm Warn only for Dev mode

saphal1998 force-pushed the lineNumberOfHooks branch from 9516688 to 5fae37c Compare December 8, 2020 18:06

saphal1998 added 2 commits December 8, 2020 23:58

Shallow merge variable names into hooks in InspectedElement

49ee150

Refactor code into react-devtools-shared

53ca0eb

vibhorgupta-gh and others added 2 commits December 13, 2020 21:41

fix: snapshots updated for inspectedElementContext-test.js

a06f8d9

Added unit tests for injectHookVariableNames

ff7b6c3

saphal1998 force-pushed the lineNumberOfHooks branch from 64f9113 to ff7b6c3 Compare December 14, 2020 19:14

Added unit tests for injectHookVariableNames

31ce9bc

vibhorgupta-gh force-pushed the lineNumberOfHooks branch from 8efb506 to 31ce9bc Compare December 16, 2020 20:19

vibhorgupta-gh added 2 commits December 17, 2020 23:39

Added snapshot tests for injectHookVariableNames

d082e69

wip: e2e test

3c08354

vibhorgupta-gh requested a review from bvaughn December 17, 2020 23:29

This was referenced Apr 5, 2021

Named hooks: MVP support facebook/react#16474

Closed

[DevTools] Named hooks facebook/react#19097

Closed

bvaughn mentioned this pull request Jun 8, 2021

DevTools: Show hook names based on variable usage facebook/react#21641

Merged

14 tasks

bvaughn reviewed Jun 8, 2021

View reviewed changes

packages/react-devtools-shared/src/utils.js Show resolved Hide resolved

bvaughn reviewed Jun 8, 2021

View reviewed changes

mknmohit approved these changes Jun 25, 2021

View reviewed changes

bvaughn closed this Jul 1, 2021

bvaughn deleted the lineNumberOfHooks branch July 1, 2021 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting hook variable names in devtools extension #115

Supporting hook variable names in devtools extension #115

vibhorgupta-gh commented Nov 21, 2020 •

edited by saphal1998

Loading

bvaughn left a comment

vibhorgupta-gh commented Dec 17, 2020 •

edited

Loading

mknmohit commented May 5, 2021

ankursnh023 commented May 19, 2021

bvaughn commented Jun 7, 2021 •

edited

Loading

bvaughn commented Jun 7, 2021

bvaughn commented Jun 7, 2021 •

edited

Loading

bvaughn Jun 8, 2021 •

edited

Loading

saphal1998 commented Jun 11, 2021

bvaughn commented Jun 11, 2021

bvaughn commented Jun 17, 2021 •

edited

Loading

mknmohit Jun 25, 2021

mknmohit Jun 25, 2021

mknmohit Jun 25, 2021

bvaughn commented Jul 1, 2021

cmacdonnacha commented Jul 1, 2021

bvaughn commented Jul 1, 2021

saphal1998 commented Jul 1, 2021

Supporting hook variable names in devtools extension #115

Supporting hook variable names in devtools extension #115

Conversation

vibhorgupta-gh commented Nov 21, 2020 • edited by saphal1998 Loading

bvaughn left a comment

Choose a reason for hiding this comment

vibhorgupta-gh commented Dec 17, 2020 • edited Loading

mknmohit commented May 5, 2021

ankursnh023 commented May 19, 2021

bvaughn commented Jun 7, 2021 • edited Loading

bvaughn commented Jun 7, 2021

bvaughn commented Jun 7, 2021 • edited Loading

bvaughn Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

saphal1998 commented Jun 11, 2021

bvaughn commented Jun 11, 2021

bvaughn commented Jun 17, 2021 • edited Loading

mknmohit Jun 25, 2021

Choose a reason for hiding this comment

mknmohit Jun 25, 2021

Choose a reason for hiding this comment

mknmohit Jun 25, 2021

Choose a reason for hiding this comment

bvaughn commented Jul 1, 2021

cmacdonnacha commented Jul 1, 2021

bvaughn commented Jul 1, 2021

saphal1998 commented Jul 1, 2021

vibhorgupta-gh commented Nov 21, 2020 •

edited by saphal1998

Loading

vibhorgupta-gh commented Dec 17, 2020 •

edited

Loading

bvaughn commented Jun 7, 2021 •

edited

Loading

bvaughn commented Jun 7, 2021 •

edited

Loading

bvaughn Jun 8, 2021 •

edited

Loading

bvaughn commented Jun 17, 2021 •

edited

Loading