[Fresh] Make all errors recoverable #17438
Conversation
|
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 67b6791:
|
Details of bundled changes.Comparing: 54f6673...67b6791 react-refresh
Size changes (experimental) |
Details of bundled changes.Comparing: 54f6673...67b6791 react-refresh
Size changes (stable) |
| hasLoggedError = true; | ||
| warningWithoutStack( | ||
| false, | ||
| 'React DevTools encountered an error: %s', |
There was a problem hiding this comment.
This warning is a bit misleading, and might result in people filing bugs against DevTools that should mention Fresh instead.
There was a problem hiding this comment.
It’s just copy paste from other “DevTools hooks”. We can change the message in all of them? I don’t think this one is particularly special other than the fact that it happens to be used by Fresh today.
Technically there’s nothing preventing DevTools from using this one too — in fact I think it might help us remove some fragile logic in the renderer. (For new versions at least.) In that case we’d need to remove the DEV-only flag check though.
In either case the issue would be filed in the same repo.
There was a problem hiding this comment.
In other words, we could apply the same argument to the “commit root” hook which has the same message but is used by both DevTools and Fresh.
There was a problem hiding this comment.
I know it was copy pasted from the previous one, but in this particular case it's always incorrect.
There was a problem hiding this comment.
I don't understand so much pushback against a wording suggestion 😄
There was a problem hiding this comment.
I don't think it matters but I'm happy to stamp a PR that changes it :-) I don't mean to "push back", I was only explaining my reasoning for why I left it as is. I'm sorry if that explanation isn't useful.
There was a problem hiding this comment.
It's cool. Thanks for the follow up PR.
* [Fresh] Detect root updates more reliably * [Fresh] Use WeakMap for root elements * [Fresh] Make initial failures recoverable too * Fix DevTools check * Fix wrong flow type
* [Fresh] Detect root updates more reliably * [Fresh] Use WeakMap for root elements * [Fresh] Make initial failures recoverable too * Fix DevTools check * Fix wrong flow type
Summary: The `hasUnrecoverableErrors` function has been [hardcoded](https://github.com/facebook/react/blob/f38c22b244086f62ae5ed851b6ed17029ec44be5/packages/react-refresh/src/ReactFreshRuntime.js#L602) to always return false for the past 5 years, since React Refresh [can recover from all errors](facebook/react#17438). This hardcoding was introduced in react-refresh v0.7.1, and RN currently uses v0.14.2. ## Changelog: [INTERNAL] [REMOVED] - Remove unreachable if-condition in refresh logic Pull Request resolved: #45296 Test Plan: Fast Refresh should still work as expected. Reviewed By: NickGerleman Differential Revision: D59405648 Pulled By: arushikesarwani94 fbshipit-source-id: 6fefedb484eeab032028d738b48ac936a9044cb0
The logic for recovering from errors was a bit shaky because we tried to guess what was last scheduled from the commit event. That was insufficient because:
The first is a current limitation of Fast Refresh which is annoying. The second leads to some edge cases where it can't reliably recover even on updates.
I'm changing the strategy to introduce a first-class DevTools hook that fires when a root gets scheduled an element. We track these in Fresh runtime in a WeakMap (if not available, retrying is off completely). This lets us recover from both initial and update errors, and also tell reliably what was the last scheduled element.
Because this DevTools hook is new, I had to add some checks. I also cleaned up an old instrumentation file that existed for a similar purpose but is unused. Also added a re-entrancy check to the Fresh runtime. Should be unnecessary but I'm starting to rely on it not being re-entrant.