feat(react): prevent recursive exposing fallback when fallback throw error #1409
Conversation
🦋 Changeset detectedLatest commit: 17817fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
People can be co-author:
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: +277 B (+0.44%) Total Size: 63.8 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #1409 +/- ##
==========================================
+ Coverage 79.15% 79.40% +0.25%
==========================================
Files 67 67
Lines 566 573 +7
Branches 125 126 +1
==========================================
+ Hits 448 455 +7
Misses 113 113
Partials 5 5
|
manudeli
changed the title
Co-authored-by: Lee HyunJae (whale) <[email protected]>
Co-authored-by: lucas0530 <[email protected]>
Co-authored-by: HYUNGU KANG <[email protected]>
…e-fallback-throw-error
manudeli
force-pushed
the
feat/prevent-recursive-fallback-throw-error
branch
from
6973ced to
eeb5157
Compare
I agree that it would be good to discuss whether such an interface is needed, but I think adding such an interface would be better discussed somewhere else than this Pull Request. Make issue or pull request please. |
@gwansikk I think developers will expect a similar implementation, as the official React documentation introduces ErrorBoundary as an interface that replaces try catch. 
try catchtry {
try {
throw new Error('throwed in try');
} catch {
throw new Error('throwed in catch inside'); // infinite loop does not occur
}
} catch (e) {
console.log(e); // caught
}Expected ErrorBoundary<ErrorBoundary fallback={({ error }) => console.log(error)}> // I thought the throw that occurred in fallback(catch) should be caught at the top.
<ErrorBoundary fallback={() => { throw new Error('throwed in fallback(catch)') }}> // I thought an infinite loop shouldn't happen
{createElement(() => {
throw new Error('throwed in children(try)')
})}
</ErrorBoundary>
</ErrorBoundary>
I think configurable looping fallback is not good idea. I think the more settings a library user has to know, the harder it becomes to understand. Also, exposing fallbacks loop doesn't have the advantage of being configurable. Since this is a breaking change, it seems like it would be easier to migrate by providing a prop that guarantees the same behavior as the existing ErrorBoundary. However, it would be nice if this prop had I think it would be better to continue this in another Pull Request/issue to provide a configurable prop to make it behave like the existing ErrorBoundary, so I'll merge it first. 👍 |
Great, I completely understand and agree! |
🚧 This PR could make a huge difference, so please help us make sure this change gets a very thorough review. 🚧
Problem: ErrorBoundary's fallback can't be treated by parent ErrorBoundary
Thrown Error in fallback will be caught by ErrorBoundary self and then expose fallback recursively 🥲
Solution: When we meet thrown error in fallback of ErrorBoundary wrap it as InternalFallbackError, re-throw InternalFallbackError.fallbackError, if it is InternalFallbackError
Thrown Error in fallback will be caught by parent ErrorBoundary 👍
PR Checklist