DevTools: Show hook names based on variable usage

[bvaughn]

Builds on top of MLH-Fellowship#115. Special thanks to @saphal1998 and @VibhorCodecianGupta for working on this feature.

MLH-Fellowship#115 is several months old (my fault) and a lot has changed. In addition to that, there were some unanswered questions still as of the end of last year when I last reviewed it. This PR stacks on top of that one and tracks the remaining work (listed below) which I will be addressing shortly.

---

---

Misc

• Rewrite/marge changes to InspectedElementContext (which changed dramatically in Update DevTools to use getCacheForType API #20548 and DevTools: Restore inspect-element bridge optimizations #20789). (Split loading and parsing names into their own separate Suspense cache and return an array of names rather than mutating the inspected element objects.)
• Move code that references browser APIs (e.g. chrome.*) from react-devtools-shared package and it put into the react-devtools-extensions package.
• Add a user preference (enabled disabled by default) to toggle named hooks support (in case performance degrades poorly for large sites).
• Fix updates to inspected elements erasing the hook names. (For some reason, the Suspense cache is returning null after updates for certain cases only like on Facebook.com. Can't reproduce in a simple test harness though.)
• Replace the array of hook names with a map of hook (id?) to hook name (to better support nested/custom hooks).
• Add options to parse hook names for only a single (currently selected) component.

Embedded static resources

• Remove hard-coded react-devtools-shared/src/__tests__/resources/bundle.js file in favor of a pre-test script that generates this bundle (but does not check it into the Git repository).
• Remove hard-coded packages/react-devtools-extensions/mappings.wasm file from source. (Even if we do a just-in-time copy from node_modules as part of the build process for the browser extension buidls.)

Feature flags

• Make sure all named hooks logic is gated behind a flag in 'react-devtools-feature-flags'.
• Make sure feature is disabled entirely for standalone (React Native) and inline packages.

Testing

• Fix failing unit tests in parseHookNames-test.js (currently disabled via xit(...))
• Fix failing unit tests on CI (e.g. Job 334882) which seem to be related to either a node modules resolution issue or caching. - Install nested packages from Yarn cache before running build tests #21779
• Create a couple of test fixtures (e.g. static site without source map, code sandbox, create-react-app, maybe a parcel app)
• Verify named hooks and enableProfilerChangedHookIndices features are compatible.

[bvaughn]

TypeError: _sourceMap.SourceMapConsumer.with is not a function

I think this error on CI means that Yarn resolutions aren't working correctly and the tests are being run with the wrong version of "source-map". I'll have to dig into that.

[bvaughn]

The failing tests (in parseHookNames-test) may be due to source map generation or loading somehow. I can use the exact same source code on Code Sandbox with the built extension and it works okay, but trying it in the test fails.

I also verified that Chrome loads both inline and external source maps correctly for the compiled files, so it seems most likely to be something related to how Jest or the test environment work.

Looking at the source location info returned by "react-debug-tools", the line and column numbers seem correct. So I think the problem is somehow related to the source map conversion step.

---

Consider this example code:

import React, {useState} from 'react';

function Example() {
  const [count, setCount] = useState(0);

  return (
    <div>
      <p>You clicked {count} times</p>
      <button onClick={() => setCount(count + 1)}>Click me</button>
    </div>
  );
}

In the "source-map" library, looking at this part of this code I'm seeing a mismatch between the "needle" (where in the code our Error stack parsing library says the source is) and the "mapping" (where the source maps parsing library thinks it is):

    if (mapping) {
      if (mapping.generatedLine === needle.generatedLine) {
        let source = util.getArg(mapping, "source", null);
        if (source !== null) {
          source = this._absoluteSources.at(source);
        }

        let name = util.getArg(mapping, "name", null);
        if (name !== null) {
          name = this._names.at(name);
        }

        return {
          source,
          line: util.getArg(mapping, "originalLine", null),
          column: util.getArg(mapping, "originalColumn", null),
          name
        };
      }
    }

    return {
      source: null,
      line: null,
      column: null,
      name: null
    };

In the browser, I see that both needle and mapping line up but in Jest they don't line up. In the browser, the originalLine (in the mapping) aligns with the source code and the generatedLine for both the needle and the mapping align with the compiled code. However in Jest/Node, the generatedLine for the needle (13) aligns with the source code while the generatedLine of the mapping (8) aligns with the compiled code (and the originalLine is null).

Edit: If I disable the "minify" preset when compiling, the originalLine is not null (but it is also incorrect).

---

The following reduced test case works for me in Node:

const encoded = `eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIkV4YW1wbGUuanMiXSwibmFtZXMiOlsiRXhhbXBsZSIsImNvdW50Iiwic2V0Q291bnQiXSwibWFwcGluZ3MiOiI7Ozs7Ozs7O0FBU0E7Ozs7OztBQVRBOzs7Ozs7OztBQVdPLFNBQVNBLE9BQVQsR0FBbUI7QUFDeEIsUUFBTSxDQUFDQyxLQUFELEVBQVFDLFFBQVIsSUFBb0IscUJBQVMsQ0FBVCxDQUExQjtBQUVBLHNCQUNFLHVEQUNFLHdEQUFnQkQsS0FBaEIsV0FERixlQUVFO0FBQVEsSUFBQSxPQUFPLEVBQUUsTUFBTUMsUUFBUSxDQUFDRCxLQUFLLEdBQUcsQ0FBVDtBQUEvQixnQkFGRixDQURGO0FBTUQ7O2VBRWNELE8iLCJzb3VyY2VzQ29udGVudCI6WyIvKipcbiAqIENvcHlyaWdodCAoYykgRmFjZWJvb2ssIEluYy4gYW5kIGl0cyBhZmZpbGlhdGVzLlxuICpcbiAqIFRoaXMgc291cmNlIGNvZGUgaXMgbGljZW5zZWQgdW5kZXIgdGhlIE1JVCBsaWNlbnNlIGZvdW5kIGluIHRoZVxuICogTElDRU5TRSBmaWxlIGluIHRoZSByb290IGRpcmVjdG9yeSBvZiB0aGlzIHNvdXJjZSB0cmVlLlxuICpcbiAqIEBmbG93XG4gKi9cblxuaW1wb3J0IFJlYWN0LCB7dXNlU3RhdGV9IGZyb20gJ3JlYWN0JztcblxuZXhwb3J0IGZ1bmN0aW9uIEV4YW1wbGUoKSB7XG4gIGNvbnN0IFtjb3VudCwgc2V0Q291bnRdID0gdXNlU3RhdGUoMCk7XG5cbiAgcmV0dXJuIChcbiAgICA8ZGl2PlxuICAgICAgPHA+WW91IGNsaWNrZWQge2NvdW50fSB0aW1lczwvcD5cbiAgICAgIDxidXR0b24gb25DbGljaz17KCkgPT4gc2V0Q291bnQoY291bnQgKyAxKX0+Q2xpY2sgbWU8L2J1dHRvbj5cbiAgICA8L2Rpdj5cbiAgKTtcbn1cblxuZXhwb3J0IGRlZmF1bHQgRXhhbXBsZTtcbiJdfQ`;

const {SourceMapConsumer} = require('source-map');

if (__BROWSER__) {
  const wasmMappingsURL = chrome.extension.getURL('mappings.wasm');
  SourceMapConsumer.initialize({'lib/mappings.wasm': wasmMappingsURL});
}

SourceMapConsumer.with(
  sourceMapContents,
  null,
  sourceConsumer => {
    console.log('Searching for line: 13, column: 29 ...');
    const data = sourceConsumer.originalPositionFor({
      line: 24,
      column: 49,
    });
    console.log('Found:', data);
  },
);

I think the core problem then is that the line and column numbers being returned by "react-debug-tools" are wrong for Node vs the browser. (They correspond to the source code rather than the compiled code.)

---

Found another test case that breaks in a different way. Given the following source:

/**
 * 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 * as React from 'react';
import {Fragment, useCallback, useState} from 'react';

function ListItem({item}) {
  const handleDelete = useCallback(() => {
    removeItem(item);
  }, [item, removeItem]);

  return null;
}

export default function List(props) {
  const [items] = useState([]); // 22, 19

  return items.map((item, index) => <ListItem key={index} item={item} />);
}

And the following compiled JS:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = List;

var React = _interopRequireWildcard(require("react"));

function _getRequireWildcardCache() { if (typeof WeakMap !== "function") return null; var cache = new WeakMap(); _getRequireWildcardCache = function () { return cache; }; return cache; }

function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } if (obj === null || typeof obj !== "object" && typeof obj !== "function") { return { default: obj }; } var cache = _getRequireWildcardCache(); if (cache && cache.has(obj)) { return cache.get(obj); } var newObj = {}; var hasPropertyDescriptor = Object.defineProperty && Object.getOwnPropertyDescriptor; for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { var desc = hasPropertyDescriptor ? Object.getOwnPropertyDescriptor(obj, key) : null; if (desc && (desc.get || desc.set)) { Object.defineProperty(newObj, key, desc); } else { newObj[key] = obj[key]; } } } newObj.default = obj; if (cache) { cache.set(obj, newObj); } return newObj; }

/**
 * 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
 */
function ListItem({
  item
}) {
  const handleDelete = (0, React.useCallback)(() => {
    removeItem(item);
  }, [item, removeItem]);
  return null;
}

function List(props) {
  const [items] = (0, React.useState)([]);
  return items.map((item, index) => /*#__PURE__*/React.createElement(ListItem, {
    key: index,
    item: item
  }));
}
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIlRlbXAuanMiXSwibmFtZXMiOlsiTGlzdEl0ZW0iLCJpdGVtIiwiaGFuZGxlRGVsZXRlIiwicmVtb3ZlSXRlbSIsIkxpc3QiLCJwcm9wcyIsIml0ZW1zIiwibWFwIiwiaW5kZXgiXSwibWFwcGluZ3MiOiI7Ozs7Ozs7QUFTQTs7Ozs7O0FBVEE7Ozs7Ozs7O0FBWUEsU0FBU0EsUUFBVCxDQUFrQjtBQUFDQyxFQUFBQTtBQUFELENBQWxCLEVBQTBCO0FBQ3hCLFFBQU1DLFlBQVksR0FBRyx1QkFBWSxNQUFNO0FBQ3JDQyxJQUFBQSxVQUFVLENBQUNGLElBQUQsQ0FBVjtBQUNELEdBRm9CLEVBRWxCLENBQUNBLElBQUQsRUFBT0UsVUFBUCxDQUZrQixDQUFyQjtBQUlBLFNBQU8sSUFBUDtBQUNEOztBQUVjLFNBQVNDLElBQVQsQ0FBY0MsS0FBZCxFQUFxQjtBQUNsQyxRQUFNLENBQUNDLEtBQUQsSUFBVSxvQkFBUyxFQUFULENBQWhCO0FBRUEsU0FBT0EsS0FBSyxDQUFDQyxHQUFOLENBQVUsQ0FBQ04sSUFBRCxFQUFPTyxLQUFQLGtCQUFpQixvQkFBQyxRQUFEO0FBQVUsSUFBQSxHQUFHLEVBQUVBLEtBQWY7QUFBc0IsSUFBQSxJQUFJLEVBQUVQO0FBQTVCLElBQTNCLENBQVA7QUFDRCIsInNvdXJjZXNDb250ZW50IjpbIi8qKlxuICogQ29weXJpZ2h0IChjKSBGYWNlYm9vaywgSW5jLiBhbmQgaXRzIGFmZmlsaWF0ZXMuXG4gKlxuICogVGhpcyBzb3VyY2UgY29kZSBpcyBsaWNlbnNlZCB1bmRlciB0aGUgTUlUIGxpY2Vuc2UgZm91bmQgaW4gdGhlXG4gKiBMSUNFTlNFIGZpbGUgaW4gdGhlIHJvb3QgZGlyZWN0b3J5IG9mIHRoaXMgc291cmNlIHRyZWUuXG4gKlxuICogQGZsb3dcbiAqL1xuXG5pbXBvcnQgKiBhcyBSZWFjdCBmcm9tICdyZWFjdCc7XG5pbXBvcnQge0ZyYWdtZW50LCB1c2VDYWxsYmFjaywgdXNlU3RhdGV9IGZyb20gJ3JlYWN0JztcblxuZnVuY3Rpb24gTGlzdEl0ZW0oe2l0ZW19KSB7XG4gIGNvbnN0IGhhbmRsZURlbGV0ZSA9IHVzZUNhbGxiYWNrKCgpID0+IHtcbiAgICByZW1vdmVJdGVtKGl0ZW0pO1xuICB9LCBbaXRlbSwgcmVtb3ZlSXRlbV0pO1xuXG4gIHJldHVybiBudWxsO1xufVxuXG5leHBvcnQgZGVmYXVsdCBmdW5jdGlvbiBMaXN0KHByb3BzKSB7XG4gIGNvbnN0IFtpdGVtc10gPSB1c2VTdGF0ZShbXSk7XG5cbiAgcmV0dXJuIGl0ZW1zLm1hcCgoaXRlbSwgaW5kZXgpID0+IDxMaXN0SXRlbSBrZXk9e2luZGV4fSBpdGVtPXtpdGVtfSAvPik7XG59XG4iXX0=
//# sourceURL=Temp.js

In Jest, "react-debug-tools" initially identifies the line number of the useState hook as 22 (source location) and the call to consumer.originalPositionFor() returns line generatedLine == 22 and originalLine == 13 which is unexpected. I guess the problem, again, is that the starting line number corresponds to the original source code rather than the compiled source code.

Is Node automatically applying the source map? If I remove the //# sourceMappingURL comment, the line number is what I'd expect it to be.

---

Final edit 1: This seems to be jest (jest-runner maybe?) auto-configuring Errors to account for source maps (see 9f4627f).

Final edit 2: Tests were passing on Friday and are failing again (mismatched stack). For whatever reason, the change made in 9f4627f stopped working over the weekend.

Final edit 3: Alternate approach pushed in 09fe69c to detect if source maps are being auto-applied to Error stack frames and skip converting them if so. I don't know if this will fix unit tests, though it does seem to be an important step.

[bvaughn]

At some point, Jest was automatically modifying Error stack frames to take source-maps into account. Probably this:
https://github.com/facebook/jest/blob/edf28035b7c8d5faeb50c2a4fc5fdb496351c720/packages/jest-runner/src/runTest.ts#L191-L207

Then when I explicitly deleted the Error.prepareStackTrace property, it stopped– and then today, it started doing it again.

I'm not sure why it stopped. (I thought it was my modification of the Error object.) I think it started again when I connected Node to the v8 debugger. I'm not sure why it continued doing it, even after the debugger was closed/disconnected.

This (750bf77) is how I attempted to work around the issue. More specifically, detecting the case where source maps have already been applied to stack frames:
https://github.com/facebook/react/blob/750bf776c4ea6d7c4e83c824306794ecdbf3ca9a/packages/react-devtools-extensions/src/ErrorTester.js#L18-L27

In that case, I skip translating the line numbers:
https://github.com/facebook/react/blob/750bf776c4ea6d7c4e83c824306794ecdbf3ca9a/packages/react-devtools-extensions/src/parseHookNames.js#L242-L247

But it's unsatisfactory, because it means that my tests don't/can't cover the source map parsing logic.

[bvaughn]

Okay I think this is nearing code review readiness. Biggest problem at the moment is the failing unit tests on CI (that pass locally). Would welcome a hand if anyone has ideas for this.

The new parseHookNames-test is passing locally (for me and other core members) but fails on CI (e.g. Job 334882). My best guess is that this is a Yarn installation issue or a node modules resolution issue for the workspace (or possibly something to do with our module caching, though I think I've ruled that out by explicitly invalidating the cache).

What we're seeing right now is that tests fail with the following error:

    TypeError: _sourceMap.SourceMapConsumer.with is not a function

      305 |       // Parse and extract the AST from the source map.
      306 |       promises.push(
    > 307 |         SourceMapConsumer.with(
          |                                        ^
      308 |           sourceMapContents,
      309 |           null,
      310 |           (sourceConsumer: SourceConsumer) => {

      at packages/react-devtools-extensions/src/parseHookNames.js:307:40
          at Map.forEach (<anonymous>)
      at parseSourceAST (packages/react-devtools-extensions/src/parseHookNames.js:302:28)
      at packages/react-devtools-extensions/src/parseHookNames.js:100:28

Which indicates that source-map 0.6.1 is being used (from the root node_modules directory) rather than version 0.8.0-beta.0 (the dependency specified by react-devtools-extensions). I'm not sure why CI is seeing different results than my local tests though.

I think I've ruled out the version of Node as a potential cause by verifying that tests pass locally for me with 12, 14, and 16. Andrew was also able to confirm that they pass for him on a clean checkout of this branch. (I've also removed and reinstalled node_modules a few times.)

[chigia001]

The problem is because CircleCi config and node_modules resolution

At setup job, we install the yarn workspace's node_modules + each package's node_modules structure. But only the root node_modules folder is persist to CircleCI's Workspace and CircleCi's cache, no packages/*/node_modules persist anywhere

when running the test in yarn_test_build---project=devtools -r=experimental job, Circle is cloning a fresh directory(or server, I don't really know) and then we attach the CircleCi's workspace and cache back to this new directory (those 2 file directory is generated from setup step). But because those 2 components don't incude react/packages/react-devtools-extensions/node_modules folder so when when the test is run, node.js try to find the source-map package in parent's node_modules folder, which reside at react's root/node_modules. But that version is 0.6.1 on my machine.

EDIT: you can re-produce this by remove react/packages/react-devtools-extensions/node_modules folder on your local machine and run the test step again

[bvaughn]

@chigia001 Thank you for the explanation! That makes sense conceptually, although I had hoped that the change I committed in ede48c4 would fix the test if this was the case (since it explicitly installs the nested node_modules before running DevTools tests). Maybe I'm still misunderstanding something about how this stuff is configured though!

EDIT: you can re-produce this by remove react/packages/react-devtools-extensions/node_modules folder on your local machine and run the test step again

Right. I knew the issue was that the wrong version of source-map is being loaded. Just wasn't sure of why (runtime resolution issue or Yarn installation/caching issue).

[bvaughn]

After fixing CI (via #21779) tests are passing, but I see something else that concerns me.

Here are the results of running DevTools tests on CI:

And here are the results of running them locally:

Why is CI only running 2 of the 27 tests? The strange thing is, it's pulling them from both packages (react-devtools-shared and react-devtools-extensions) but it seems to be pulling only one test per directory.

Filed a follow up task for this (#21780) since the behavior is not specific to this branch or PR.

[bvaughn]

I am going to land this PR now with the feature only enabled for internal builds of the extension. I'd like to get people trying the feature out and reporting bugs for a bit before I release it to the larger browser stores.

Worth keeping in mind that this feature can be slow for large apps, which is why it's off by default, but the follow up task #21782 should hopefully help with this.

comet-named-hooks-slow-Kapture.2021-07-01.at.13.53.47.mp4

[motiz88]

If the compiled code is a bundle, it can consist of multiple modules - what guarantees that sources[0] is the correct module for the hook we'll be looking up? The originalPositionFor call in findHookNames above returns a source property which we currently ignore; I think the correct thing to do is to lazily parse/cache the source at that point rather than make this assumption here.

[motiz88]

Diving into this in #21790, need help debugging my basic test setup though.

[bvaughn]

We also only support one source map:
https://github.com/facebook/react/blob/ed6c091fe961a3b95e956ebcefe8f152177b1fb7/packages/react-devtools-extensions/src/parseHookNames.js#L224-L230

But maybe that's less of a common concern?

I've filed #21794 for the main issue mentioned here.

[motiz88]

I'm not sure what multiple source maps would even mean - a standard source map represents a single, contiguous, compiled file from start to finish. If the browser is executing a file with multiple sourceMappingURL comments in it, it's basically a bug in the bundler and should be fixed there. Our best chance is to emulate e.g. Chrome's behaviour if this happens: from a cursory look at V8's parser, I think the last one wins.

[bvaughn]

You can see this on Code Sandbox, for example. IIR it generates a single bundle and then injects multiple (inline) source maps.

Our best chance is to emulate e.g. Chrome's behaviour if this happens: from a cursory look at V8's parser, I think the last one wins.

But yeah, this makes sense 👍🏼

So we're actually doing the opposite now? I think last one wouldn't work for Code Sandbox then, since what we're doing works.

[motiz88]

Do you have a repro for multiple sourceMappingURLs on CodeSandbox? I did not see multiple sourceMappingURL comments in a trivial React sandbox I created just now. I'd love to investigate further and send them a PR if necessary.

[bvaughn]

If I find one again, I'll leave a comment here. I remember seeing one when testing Code Sandbox specifically, but spot checking a few of my recen test apps– I don't see it.

[motiz88]

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.

[bvaughn]

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.

[motiz88]

A non-perf reason to invest in #21782 ( = derive metadata from the AST at build time) is that there's no single set of plugins we can pass here that will work for all use cases.

1. If the source code is written in Flow, it likely will not parse as TypeScript.
2. People using experimental syntax in their code will run into parse errors here. There used to be a catch-all '*' option you could pass to Babel and enable all plugins, but that was removed when plugins with conflicting syntax were introduced (Flow / TypeScript, divergent proposals for decorators/pipeline operators etc)

[bvaughn]

Another good point. I also wonder about perf and file size impact for enabling all plugins, even if that were still an option.

I've filed #21793 as a follow up for this too.

[vibhorgupta-gh]

@bvaughn thanks for driving this all the way! We haven't been able to look into any of the recent issues due to work commitments. Super excited to see this getting shipped! 🚀

[bvaughn]

No problem 👍🏼