Remove invokeGuardedCallback and replay trick #28515
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
approved these changes
Mar 7, 2024
| !originalError._suppressLogging | ||
| ) { | ||
| // If suppressed, let the flag carry over to the original error which is the one we'll rethrow. | ||
| originalError._suppressLogging = true; |
There was a problem hiding this comment.
I forget how this was consumed but do we still want to support this log suppression? Was it useful for reasons outside of the double error reporting? I think this is used by some frameworks to avoid the double log by customizing the window error handling to not warn on errors-as-control flow like redirect and notFound
There was a problem hiding this comment.
I think it's maybe to avoid logging the globally caught error if you've already handled it.
gaearon
approved these changes
Mar 7, 2024
rickhanlonii
approved these changes
Mar 8, 2024
sebmarkbage
force-pushed
the
rminvokeguardedcallback
branch
2 times, most recently
from
de66616
to
fe246c9
Compare
sebmarkbage
commented
Mar 9, 2024
| it('are properly handled for layout effect creation', async () => { | ||
| let useLayoutEffectShouldThrow = false; | ||
|
|
||
| function ThrowsInLayoutEffect() { | ||
| function ThrowsInLayoutEffect({unused}) { |
There was a problem hiding this comment.
These tests used to only run in dev.
In prod we don't disable logs during component stack generation, so when the component stack gets generated it ends up logging again. We can avoid that by ensure component stacks error before the log. E.g. by destructuring props.
sebmarkbage
commented
Mar 9, 2024
| @@ -3267,7 +3267,7 @@ describe('ReactDOMFizzServer', () => { | |||
| }); | |||
|
|
|||
| // Partially render A, but yield before the render has finished | |||
| await waitFor(['Oops!', 'Oops!']); | |||
| await waitFor(['Oops!']); | |||
There was a problem hiding this comment.
This one is weird because it used to log twice in both DEV and prod for two different reasons.
In DEV it used to log twice because we did the replay trick which I now removed.
In prod it used to log twice because the component stack generation runs the function again and we don't disable Scheduler.log in prod during this generation - only in dev. To avoid this scenario I use props destructuring so it errors earlier instead.
sebmarkbage
force-pushed
the
rminvokeguardedcallback
branch
5 times, most recently
from
b8b7566
to
eb6c0cc
Compare
sebmarkbage
commented
Mar 10, 2024
| @@ -56,7 +56,8 @@ describe('ReactDOMConsoleErrorReporting', () => { | |||
|
|
|||
| describe('ReactDOMClient.createRoot', () => { | |||
| it('logs errors during event handlers', async () => { | |||
| spyOnDevAndProd(console, 'error'); | |||
There was a problem hiding this comment.
I had to switch to manual mocking. The spyOnDevAndProd helper doesn't work in a bunch of cases and I have no idea why.
There was a problem hiding this comment.
I guess spyOn also calls the inner one. So in these cases it's because we're no longer ignoring uncaught error in logs.
|
It's not quite that easy it turns out. Added a bunch of TODOs. |
sebmarkbage
commented
Mar 10, 2024
| @@ -7,12 +7,8 @@ module.exports = function shouldIgnoreConsoleError( | |||
| ) { | |||
There was a problem hiding this comment.
We need to get this file down to zero because it's covering up a bunch of quirks.
sebmarkbage
force-pushed
the
rminvokeguardedcallback
branch
7 times, most recently
from
bfa05cd
to
62c0201
Compare
This is pretty simple now.
Since we no longer get the error printed by the browser automatically we have to manually print the error with an addendum which contains component stacks.
sebmarkbage
force-pushed
the
rminvokeguardedcallback
branch
from
62c0201
to
aae244d
Compare
|
There's no way to silence the logs when an error boundary catches it now but I have a follow up to change a few other things and introduce a possible way to silence it, but it can't be onerror that does it which is a breaking change. |
sebmarkbage
force-pushed
the
rminvokeguardedcallback
branch
from
81f7250
to
8ed9d7c
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Mar 12, 2024
We broke the ability to "break on uncaught exceptions" by adding a try/catch higher up in the scheduling. We're giving up on fixing that so we can remove the replay trick inside an event handler. The issue with that approach is that we end up double logging a lot of errors in DEV since they get reported to the page. It's also a lot of complexity around this feature. DiffTrain build for [89021fb](89021fb)
gnoff
added a commit
to gnoff/next.js
that referenced
this pull request
Mar 25, 2024
- facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
gnoff
added a commit
to gnoff/next.js
that referenced
this pull request
Mar 25, 2024
- facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
We broke the ability to "break on uncaught exceptions" by adding a try/catch higher up in the scheduling. We're giving up on fixing that so we can remove the replay trick inside an event handler. The issue with that approach is that we end up double logging a lot of errors in DEV since they get reported to the page. It's also a lot of complexity around this feature.
hoxyq
added a commit
that referenced
this pull request
Apr 15, 2024
Full list of changes: * Look for a ReactMemoCacheSentinel on state ([gsathya](https://github.com/gsathya) in [#28831](#28831)) * Use use() in the Cache if available ([sebmarkbage](https://github.com/sebmarkbage) in [#28793](#28793)) * feat[devtools-fusebox]: support theme option ([hoxyq](https://github.com/hoxyq) in [#28832](#28832)) * feat[devtools]: add package for fusebox integration ([hoxyq](https://github.com/hoxyq) in [#28553](#28553)) * feat[devtools]: add method for connecting backend with custom messaging protocol ([hoxyq](https://github.com/hoxyq) in [#28552](#28552)) * Rename SECRET INTERNALS to `__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE` ([sebmarkbage](https://github.com/sebmarkbage) in [#28789](#28789)) * Flatten ReactSharedInternals ([sebmarkbage](https://github.com/sebmarkbage) in [#28783](#28783)) * feat[devtools]: ship source maps for content scripts and ignore list installHook script ([hoxyq](https://github.com/hoxyq) in [#28730](#28730)) * Track Owner for Server Components in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [#28753](#28753)) * Move ReactDOMLegacy implementation into RootFB ([sebmarkbage](https://github.com/sebmarkbage) in [#28656](#28656)) * Reland #28672: Remove IndeterminateComponent ([gnoff](https://github.com/gnoff) in [#28681](#28681)) * Remove reference to deleted <Cache> in un-linted file ([josephsavona](https://github.com/josephsavona) in [#28715](#28715)) * [be] Remove unshipped experimental <Cache> element type ([josephsavona](https://github.com/josephsavona) in [#28698](#28698)) * Revert "Remove module pattern function component support" ([rickhanlonii](https://github.com/rickhanlonii) in [#28670](#28670)) * Remove module pattern function component support ([gnoff](https://github.com/gnoff) in [#27742](#27742)) * [RTR] Enable warning flag ([jackpope](https://github.com/jackpope) in [#28419](#28419)) * Update error messages ([rickhanlonii](https://github.com/rickhanlonii) in [#28652](#28652)) * fix[devtools/ci]: split profiling cache test for different react versions and toEqual checker ([hoxyq](https://github.com/hoxyq) in [#28628](#28628)) * Guard against legacy context not being supported in DevTools fixture ([eps1lon](https://github.com/eps1lon) in [#28596](#28596)) * Use `declare const` instead of `declare var` ([kassens](https://github.com/kassens) in [#28599](#28599)) * Update isConcurrent RTR option usage ([jackpope](https://github.com/jackpope) in [#28546](#28546)) * Disable legacy context ([kassens](https://github.com/kassens) in [#27991](#27991)) * Remove invokeGuardedCallback and replay trick ([sebmarkbage](https://github.com/sebmarkbage) in [#28515](#28515)) * Remove remaining usages of ReactTestUtils in tests unrelated to `react-dom/test-util` ([eps1lon](https://github.com/eps1lon) in [#28534](#28534)) * fix[devtools/e2e]: fixed source inspection in e2e tests ([hoxyq](https://github.com/hoxyq) in [#28518](#28518)) * Devtools: Display actual pending state when inspecting `useTransition` ([eps1lon](https://github.com/eps1lon) in [#28499](#28499))
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
We broke the ability to "break on uncaught exceptions" by adding a try/catch higher up in the scheduling. We're giving up on fixing that so we can remove the replay trick inside an event handler. The issue with that approach is that we end up double logging a lot of errors in DEV since they get reported to the page. It's also a lot of complexity around this feature. DiffTrain build for commit 89021fb.
eps1lon
added a commit
to vercel/next.js
that referenced
this pull request
Apr 19, 2024
### React upstream changes - facebook/react#28643 - facebook/react#28628 - facebook/react#28361 - facebook/react#28513 - facebook/react#28299 - facebook/react#28617 - facebook/react#28618 - facebook/react#28621 - facebook/react#28614 - facebook/react#28596 - facebook/react#28625 - facebook/react#28616 - facebook/react#28491 - facebook/react#28583 - facebook/react#28427 - facebook/react#28613 - facebook/react#28599 - facebook/react#28611 - facebook/react#28610 - facebook/react#28606 - facebook/react#28598 - facebook/react#28549 - facebook/react#28557 - facebook/react#28467 - facebook/react#28591 - facebook/react#28459 - facebook/react#28590 - facebook/react#28564 - facebook/react#28582 - facebook/react#28579 - facebook/react#28578 - facebook/react#28521 - facebook/react#28550 - facebook/react#28576 - facebook/react#28577 - facebook/react#28571 - facebook/react#28572 - facebook/react#28560 - facebook/react#28569 - facebook/react#28573 - facebook/react#28546 - facebook/react#28568 - facebook/react#28562 - facebook/react#28566 - facebook/react#28565 - facebook/react#28559 - facebook/react#28508 - facebook/react#20432 - facebook/react#28555 - facebook/react#24730 - facebook/react#28472 - facebook/react#27991 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580 - facebook/react#28514 - facebook/react#28548 - facebook/react#28526 - facebook/react#28515 - facebook/react#28533 - facebook/react#28532 - facebook/react#28531 - facebook/react#28407 - facebook/react#28522 - facebook/react#28538 - facebook/react#28509 - facebook/react#28534 - facebook/react#28527 - facebook/react#28528 - facebook/react#28519 - facebook/react#28411 - facebook/react#28520 - facebook/react#28518 - facebook/react#28493 - facebook/react#28504 - facebook/react#28499 - facebook/react#28501 - facebook/react#28496 - facebook/react#28471 - facebook/react#28351 - facebook/react#28486 - facebook/react#28490 - facebook/react#28488 - facebook/react#28468 - facebook/react#28321 - facebook/react#28477 - facebook/react#28479 - facebook/react#28480 - facebook/react#28478 - facebook/react#28464 - facebook/react#28475 - facebook/react#28456 - facebook/react#28319 - facebook/react#28345 - facebook/react#28337 - facebook/react#28335 - facebook/react#28466 - facebook/react#28462 - facebook/react#28322 - facebook/react#28444 - facebook/react#28448 - facebook/react#28449 - facebook/react#28446 - facebook/react#28447 - facebook/react#24580
This was referenced Apr 19, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We broke the ability to "break on uncaught exceptions" by adding a try/catch higher up in the scheduling. We're giving up on fixing that so we can remove the replay trick inside an event handler.
The issue with that approach is that we end up double logging a lot of errors in DEV since they get reported to the page.
It's also a lot of complexity around this feature.