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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jun 7, 2021

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 #20548 and #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).

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.
  • 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.
@sizebot

This comment was marked as off-topic.

@markerikson

This comment has been hidden.

@bvaughn bvaughn marked this pull request as ready for review Jun 24, 2021
@bvaughn bvaughn marked this pull request as draft Jun 24, 2021
@bvaughn bvaughn force-pushed the bvaughn:devtools-named-hooks branch from 8f2041f to e85c47b Jun 24, 2021
bvaughn and others added 6 commits Jun 24, 2021
@bvaughn bvaughn force-pushed the bvaughn:devtools-named-hooks branch from 32ab577 to 5cb562d Jun 25, 2021
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 25, 2021

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 bvaughn force-pushed the bvaughn:devtools-named-hooks branch from 8dbf26d to 20af667 Jun 25, 2021
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 25, 2021

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.


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) {
      // mapping.generatedLine aligns with compiled code
      // needle.generatedLine aligns with source code
      // Why are they expected to match?
      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
        };
      }
    }

    // Instead this is being returned.
    return {
      source: null,
      line: null,
      column: null,
      name: null
    };

In the browser, I see that both needle and mapping line up (e.g. generatedLine === 24) but in Jest they don't line up (e.g. needle is 82 and mapping is 10).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants