Disable console.logs in the second render pass of DEV mode double render #18547
Conversation
sebmarkbage
requested review from
bvaughn,
gaearon,
trueadm,
lunaruan and
acdlite
facebook-github-bot
added
React Core Team
|
I was just debugging some internals yesterday and wished we had this. |
|
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 b7e2345:
|
|
Could we do the same for |
| @@ -149,8 +148,7 @@ describe('ReactDOMFiberAsync', () => { | |||
| describe('concurrent mode', () => { | |||
| beforeEach(() => { | |||
| jest.resetModules(); | |||
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | |||
| ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; | |||
There was a problem hiding this comment.
Can remove internal suffix now
There was a problem hiding this comment.
Yea I'll do a follow up to rename all these if I land this. I think the remaining ones are just using the let ops = [] pattern and we can port those.
Details of bundled changes.Comparing: b225d4f...b7e2345 react-art
scheduler
react-dom
react-reconciler
Size changes (stable) |
Details of bundled changes.Comparing: b225d4f...b7e2345 react-art
scheduler
react-dom
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
I think we probably could but that one was kind of so that you could break on the exception. The problem then is your logs are disabled while on your break point. That seems highly confusing. I'm not sure that's the right call. |
| ); | ||
| disableLogs(); | ||
| try { | ||
| value = renderWithHooks( |
There was a problem hiding this comment.
I could foresee some confusion if the logs don't correspond to the value that is returned. It usually doesn't matter because it's supposed to be pure, but in the case where it's not, you might use console to debug it. So maybe they should be disabled for the first pass instead of the second.
There was a problem hiding this comment.
So I was actually surprised that we used the second value. I was thinking we should ignore the second value like we do for class constructors.
There was a problem hiding this comment.
In my other PR's replaying I have to ignore it because I force an error inside the function. Similarly the react-debug-tools for Hooks debugging it's just replayed stand-alone.
Doesn't necessarily mean we have to do the same here. I guess that's how I look at these.
There was a problem hiding this comment.
It's intentional because the children are bound to the work-in-progress fiber via the queue object via the dispatch method, and renderWithHooks mutates the fiber.
It was a very confusing bug when we found it :D
There was a problem hiding this comment.
I could go either way on if the second render should process the update queue and even leave any side-effects. :)
There was a problem hiding this comment.
Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.
There was a problem hiding this comment.
I could go either way on if the second render should process the update queue and even leave any side-effects :)
It can work but it needs to include internal mutations/side-effects too. We'd have to make it so that renderWithHooks does not mutate any of the fiber fields. Or stash the result of the first pass (memoizedState, expirationTime, effectTag) before doing the second pass and then restore them.
There was a problem hiding this comment.
Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.
Oh I didn't see that. Yeah I think that would work. Just call the component again with a special dispatcher that doesn't mutate anything.
There was a problem hiding this comment.
Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.
I might be misunderstanding this comment, but if the second call has state updates already applied, then we might miss out on triggering the observable side effects in the first place (e.g. like something that happens only when props/state meet a certain criteria).
I was thinking you would disable the logs during the first pass and enable them for the second. In the case of an error, the second pass would use |
That would mean that we would have to always replay the second pass. At least as long as there's any log. Since most of the time when nothing throws or if it's just throwing a Promise, there wouldn't be a second pass. |
That seems fine? Isn't the only new case promises? |
No if we disable the logs we'd have to detect that we had silenced a log and replay - even if there's nothing thrown at all. So any little log would severely slow down debugging. We can't replay the logs later because it would give them the wrong stacks frames in the devtools. |
|
I'm confused because we already replay the methods in dev. I don't get what's getting slower. |
|
Hm. I was under the impression that we replay the whole render but I guess it's just the begin phase. Is that even safe from a resuming perspective? Seems like we'd have a lot of latent bugs there. Probably SuspenseList. |
|
It seems like these two should just use the same mechanism. If the first pass errors, then we do the double render pass in invokeGuardedCallback |
|
We can do that in a follow up if we decide to land this. For now we need to decide on this (yes or no) to unblock the production stacks PR. |
|
One fatal flaw of silencing the first pass is warning deduping. Almost none of our warnings would show up as it currently stands and also true if others use the same technique. |
sebmarkbage
changed the title
|
To follow up on this, we've changed the behavior in 18 to be more intuitive. |
This disables console.log by temporarily patching the global console object during the second render pass when we double render in strict mode in DEV.
I could go either way on this. It is confusing that it double logs warnings, React, legacy react packages or third-party ones. However, if you do have a problem due to double rendering then you kind of want logs to debug it.
I also uses this override to detect this in our Scheduler.yieldValue helper so that it automatically doesn't double log which makes it easier to write tests that test render phases. This can be done without any internals being exposed so it works to test our build outputs.
The main motivation of this is actually a different PR where I want to double render to expose stack traces. In this case it's extra strange because it's extracting the error stack from the first one that causes new warnings that are not necessarily the same. So I think we need something there. We don't necessarily have to do the same for these ones.
This is ofc not exhaustive since there are so many other side-effects than just logs and you could have other APIs that show logs in UI etc.