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

refactor[ReactDebugHooks]: support parsing stack frames when sourcemaps are available #27612

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Oct 27, 2023

Unblocks #26446.

TL;DR:

  • The main difference is isReactWrapper function implementation. Previously, we would just look at hook stack trace entry and check if functionName ends with the corresponding hook primitive name. This doesn't work with the same setup, but with sourcemaps also being present. We use Proxy for handling custom errors, here is the implementation of the Proxy for Dispatcher
    // create a proxy to throw a custom error
    // in case future versions of React adds more hooks
    const DispatcherProxyHandler = {
    get(target: DispatcherType, prop: string) {
    if (target.hasOwnProperty(prop)) {
    return target[prop];
    }
    const error = new Error('Missing method in Dispatcher: ' + prop);
    // Note: This error name needs to stay in sync with react-devtools-shared
    // TODO: refactor this if we ever combine the devtools and debug tools packages
    error.name = 'ReactDebugToolsUnsupportedHookError';
    throw error;
    },
    };
    // `Proxy` may not exist on some platforms
    const DispatcherProxy =
    typeof Proxy === 'undefined'
    ? Dispatcher
    : new Proxy(Dispatcher, DispatcherProxyHandler);

For each supported hook, we have a stub, which saves the stack trace via new Error() call. Here is useState, for example:

hookLog.push({primitive: 'State', stackError: new Error(), value: state});

At runtime, current dispatcher can be this Proxy object - DispatcherProxy, this will modify the stack trace. Here is how the corresponding stack frame looks like without sourcemaps:

{
        columnNumber: 171,
        lineNumber: 16,
        fileName: '.../build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js',
        functionName: 'Proxy.useState',
        source: '    at Proxy.useState (.../react/build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js:16:171)'
}

When sourcemaps are present, this particular frame is different, it no longer mentions primitive hook name.

{
        columnNumber: 21,
        lineNumber: 114,
        fileName: '.../build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.js',
        functionName: 'Proxy.Error',
        source: '    at Proxy.Error (.../build/oss-stable/react-debug-tools/cjs/react-debug-tools.production.js:114:21)'
}

We do this parsing to skip React's frames, in this implementation we are only looking for user-defined hooks. In order to make this logic sourcemaps independent, instead of looking at stack trace of the error, which is created at runtime (basically once hook is called), we are also looking at primitive hook's stacktraces, which are defined here:

function getPrimitiveStackCache(): Map<string, StackFrame[]> {
if (primitiveStackCache !== null) {
return primitiveStackCache;
}
// This initializes a cache of all primitive hooks so that the top
// most stack frames added by calling the primitive hook can be removed.
const cache = new Map<string, StackFrame[]>();
let readHookLog;
try {
// Use all hooks here to add them to the hook log.
Dispatcher.useContext(({_currentValue: null}: any));
Dispatcher.useState(null);
Dispatcher.useReducer((s: mixed, a: mixed) => s, null);
Dispatcher.useRef(null);
if (typeof Dispatcher.useCacheRefresh === 'function') {
// This type check is for Flow only.
Dispatcher.useCacheRefresh();
}
Dispatcher.useLayoutEffect(() => {});
Dispatcher.useInsertionEffect(() => {});
Dispatcher.useEffect(() => {});
Dispatcher.useImperativeHandle(undefined, () => null);
Dispatcher.useDebugValue(null);
Dispatcher.useCallback(() => {});
Dispatcher.useMemo(() => null);
if (typeof Dispatcher.useMemoCache === 'function') {
// This type check is for Flow only.
Dispatcher.useMemoCache(0);
}
} finally {
readHookLog = hookLog;
hookLog = [];
}
for (let i = 0; i < readHookLog.length; i++) {
const hook = readHookLog[i];
cache.set(hook.primitive, ErrorStackParser.parse(hook.stackError));
}
primitiveStackCache = cache;
return cache;
}

Notice that we are using general Dispatcher here and not DispatcherProxy. Both with or without sourcemaps stack traces contain corresponding primitive hook name in the functionName.

Also in these changes:

  • Added Flow typings for error-stack-parser via flow-typed, updated other parts of the code which were using this library accordingly.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 27, 2023
@react-sizebot
Copy link

react-sizebot commented Oct 27, 2023

Comparing: 0c63487...a43f175

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 175.09 kB 175.09 kB = 54.48 kB 54.48 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.21 kB 177.21 kB = 55.16 kB 55.16 kB
facebook-www/ReactDOM-prod.classic.js = 567.70 kB 567.70 kB = 99.96 kB 99.96 kB
facebook-www/ReactDOM-prod.modern.js = 551.56 kB 551.56 kB = 97.06 kB 97.06 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +5.36% 22.69 kB 23.90 kB +5.55% 6.18 kB 6.52 kB

Generated by 🚫 dangerJS against a43f175

@hoxyq hoxyq force-pushed the devtools/make-stack-trace-parser-sourcemaps-independent branch from 8a5c48a to 52aa81f Compare October 30, 2023 12:21
hoxyq added a commit that referenced this pull request Oct 30, 2023
… files via glob (#27627)

This script is used on CI in `yarn_lint` job. With current `glob` call
settings, it includes a bunch of build files, which are actually ignored
by listing them in `.prettierignore`. This check is not failing only
because there is no build step before it.

If you run `node ./scripts/prettier/index` with build files present, you
will see a bunch of files listed as non-formatted. These changes add a
simple logic to include all paths listed in `.prettierignore` to ignore
list of `glob` call with transforming them from gitignore-style to
glob-style.

This should unblock CI for #27612,
where `flow-typed` directory will be added, turned out that including it
in `.prettierignore` is not enough.
@hoxyq hoxyq force-pushed the devtools/make-stack-trace-parser-sourcemaps-independent branch 2 times, most recently from 2a401da to 666a6bc Compare October 30, 2023 17:24
@hoxyq hoxyq force-pushed the devtools/make-stack-trace-parser-sourcemaps-independent branch from 666a6bc to a43f175 Compare October 30, 2023 18:03
@hoxyq hoxyq changed the title refactor[ReactDebugHooks]: make stack trace parser sourcemap independent refactor[ReactDebugHooks]: support parsing stack frames when sourcemaps are available Oct 30, 2023
@hoxyq hoxyq marked this pull request as ready for review October 30, 2023 18:16
@hoxyq hoxyq requested review from kassens and motiz88 October 30, 2023 18:23
@motiz88
Copy link
Contributor

motiz88 commented Nov 6, 2023

@hoxyq and I discussed this offline. I think we should unblock #26446 differently.

The source mapping behaviour that breaks the existing logic only happens in the test environment, which suggests that #26446 creates divergence between the test and deployed environments of React DevTools. Ergo, AFAICT, we don't have the right kind of test coverage to validate this change, and should instead be looking at ways to keep the tests from breaking post-#26446 without changing any DevTools logic (e.g. by disabling source map generation for react-debug-tools).

@markerikson
Copy link
Contributor

@motiz88 anything you need from me to update #26446 ? I think I'm reading this more as "we should tweak the existing tests" or something along those lines, which suggests I don't need to alter the PR, but wanted to check.

@hoxyq
Copy link
Contributor Author

hoxyq commented Nov 7, 2023

@motiz88 anything you need from me to update #26446 ? I think I'm reading this more as "we should tweak the existing tests" or something along those lines, which suggests I don't need to alter the PR, but wanted to check.

We should continue to manually exclude react-debug-tools from having sourcemaps. The main point here is that even if we had them, they won't be included in React DevTools build with this artifact, so current tests will diverge from the real world use case.

For now lets merge your sourcemaps PR with manually excluding sourcemaps, which I will remove later when move react-debug-tools package to React DevTools' backend.

@hoxyq hoxyq closed this Nov 7, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
… files via glob (facebook#27627)

This script is used on CI in `yarn_lint` job. With current `glob` call
settings, it includes a bunch of build files, which are actually ignored
by listing them in `.prettierignore`. This check is not failing only
because there is no build step before it.

If you run `node ./scripts/prettier/index` with build files present, you
will see a bunch of files listed as non-formatted. These changes add a
simple logic to include all paths listed in `.prettierignore` to ignore
list of `glob` call with transforming them from gitignore-style to
glob-style.

This should unblock CI for facebook#27612,
where `flow-typed` directory will be added, turned out that including it
in `.prettierignore` is not enough.
@hoxyq hoxyq deleted the devtools/make-stack-trace-parser-sourcemaps-independent branch August 9, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants