Add experimental DebugTracing logger for internal use #18531
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b44b002:
|
Details of bundled changes.Comparing: b680174...b44b002 react-dom
react-art
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
Details of bundled changes.Comparing: b680174...b44b002 react-dom
react-art
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2% Size changes (experimental) |
|
Don't care about failing CI right now, just trying out an idea.
Maybe this helps cut down on the (unrelated?) noise a bit from clients like Comet who have a lot of logs. Edit It's kind of neat playing around with this on Comet, scoping my logs to e.g. the Notifications dropdown. |
bvaughn
force-pushed
the
debug-logging
branch
2 times, most recently
from
99714f2
to
7f4327f
Compare
|
Rebased and added to old/new variants. |
| @@ -488,6 +490,10 @@ export function createFiberFromTypeAndProps( | |||
| expirationTime, | |||
| key, | |||
| ); | |||
| case REACT_DEBUG_TRACE_MODE_TYPE: | |||
There was a problem hiding this comment.
Any way to make this branch DEV-only, and fall through to unknown case otherwise?
There was a problem hiding this comment.
Hm... well... we could fork the entire switch statement, but that doesn't seem good.
We could maybe...also pre-filter type, e.g.
let filteredType = type;
if (!__DEV__ && type === REACT_DEBUG_TRACE_MODE_TYPE) {
filteredType = null;
}
But neither of those really feel like improvements.
That being said, if I just conditionally export DebugTraceMode as undefined in non-DEV mode it shouldn't be reachable anyway.
|
Also need to fix SSR to pass it through. |
|
This is also missing a crucial feature: tracking of Promises. I think it's an easy fix. Here's what I hacked together: let wakeableIDs = new WeakMap();
let id = 0;
function getWakeableID(wakeable) {
if (!wakeableIDs.has(wakeable)) {
wakeableIDs.set(wakeable, id++);
}
return wakeableIDs.get(wakeable);
}
function logComponentSuspended(componentName, wakeable) {
log(
'[!] + ' + componentName + " suspended",
getWakeableID(wakeable),
wakeable.displayName,
);
wakeable.then(() => {
log('[!] Promise fulfilled',
getWakeableID(wakeable),
wakeable.displayName,
)
}, () => {
log('[!] Promise rejected',
getWakeableID(wakeable),
wakeable.displayName,
)
})
}Without this I can't tell whether something is a React or a Relay bug. |
1. Log render phase updates for useState/useReducer 2. Intercept product code console.log() calls to also print current groups
function logComponentSuspended(componentName, wakeable) {
log(
'[!] + ' + componentName + " suspended",
getWakeableID(wakeable),
wakeable.displayName,
);
wakeable.then(() => {
log('[!] Promise fulfilled',
getWakeableID(wakeable),
wakeable.displayName,
)
}, () => {
log('[!] Promise rejected',
getWakeableID(wakeable),
wakeable.displayName,
)
})
}Wakeables have Sure I can add resolve/reject tracing |
* Removed "render scheduled" message; it was the only one outside of the <Mode> and just didn't feel worth the added noise. * Added SSR pass through (and tests) * Add promise resolution/rejection logging * Removed debug mode from "react-is" package * Export for the symbol if the feature flag is off
|
Ok, back to you! |
Just the Relay ones. :D |
|
What's up with tests? |
I don't know. I still don't have a good understanding of how all of our various test combinations run and how to satisfy each of them I guess. I'll look into it... Edit Not sure why my change would cause failures in other tests. Some of the Yarn commands + test targets that failed on CI work for me locally... Going to just try re-running them. |
| @@ -39,6 +39,53 @@ describe('ReactDOMServerIntegration', () => { | |||
| resetModules(); | |||
| }); | |||
|
|
|||
| // Test pragmas don't support itRenders abstraction | |||
| if (require('shared/ReactFeatureFlags').enableDebugTracing) { | |||
There was a problem hiding this comment.
Can we align feature flag closer to component name? One way or another. I keep mistyping either of them because "trace" vs "tracing". :D
There was a problem hiding this comment.
Haha, yeah sure thing
I'm not sure why but the test failure seems to have been caused by the conditional I added to |
| @@ -488,6 +490,10 @@ export function createFiberFromTypeAndProps( | |||
| expirationTime, | |||
| key, | |||
| ); | |||
| case REACT_DEBUG_TRACING_MODE_TYPE: | |||
There was a problem hiding this comment.
Does it mean that if I somehow end up with undefined type in a stable release, after this change it would start matching REACT_DEBUG_TRACING_MODE_TYPE (which would also be undefined in stable), and thus I won't get to the catch-all default case that throws? This seems bad.
There was a problem hiding this comment.
REACT_DEBUG_TRACING_MODE_TYPE isn't undefined in stable, just React.DebugTracingMode.
There was a problem hiding this comment.
(I backed out the conditional symbol export)
There was a problem hiding this comment.
I'm still a bit confused about the plan for not shipping it in a stable release. I think currently the export would appear on React object.
Making the internal constant itself undefined wouldn't work because you don't want to match all the cases to undefined. But you can make the export from React package conditionally undefined. I also think the export should be prefixed with unstable_ in case we mess up the flags or it becomes enumerable.
packages/react/index.js
Outdated
| @@ -59,6 +59,7 @@ export { | |||
| createMutableSource, | |||
| Fragment, | |||
| Profiler, | |||
| DebugTracingMode, | |||
There was a problem hiding this comment.
Does it become enumerable (but undefined) export? If yes, it should be unstable_.
There was a problem hiding this comment.
This will never be undefined. It's just not re-exported by the .stable index. (This index file itself is never used directly.)
| @@ -11,6 +11,7 @@ import typeof * as FeatureFlagsType from 'shared/ReactFeatureFlags'; | |||
| import typeof * as ExportsType from './ReactFeatureFlags.testing.www'; | |||
|
|
|||
| export const debugRenderPhaseSideEffectsForStrictMode = false; | |||
| export const enableDebugTracing = __EXPERIMENTAL__; | |||
There was a problem hiding this comment.
Let's just make it false.
There was a problem hiding this comment.
Hopefully that won't make the test matrix gods angry.
…e they don't leak into builds
…ort to unstable_*
gaearon
changed the title
Ads debug logging capabilities (based on earlier experimental profiler branch) behind a new feature flag,
enableDebugTracing.