[Fresh] Retry failed roots on refresh#15966
Merged
gaearon merged 2 commits intofacebook:masterfrom Jun 24, 2019
Merged
Conversation
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 39b97e8...651cb27 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
react-refresh
Generated by 🚫 dangerJS |
The check wasn't very resilient because in Concurrent Mode it looks like we can get further follow-up commits even if we captured an error. So we can't reliably distinguish the case where after an error you _manually_ rendered null. Retrying on an edit after a tree failed _and_ you rendered null in the same tree seems fine. It's also very unlikely a pattern like this actually exists in the wild.
bvaughn
reviewed
Jun 24, 2019
| hook.onCommitFiberRoot(rendererID, root, priorityLevel, didError); | ||
| } else { | ||
| hook.onCommitFiberRoot(rendererID, root); | ||
| hook.onCommitFiberRoot(rendererID, root, undefined, didError); |
Contributor
There was a problem hiding this comment.
Seems like we should pass priorityLevel and didError in both cases?
Contributor
There was a problem hiding this comment.
Ignore me. I misread initially due to weird code wrapping combined with stupidity.
Collaborator
Author
There was a problem hiding this comment.
(Resolved in a chat)
bvaughn
approved these changes
Jun 24, 2019
gaearon
added a commit
to gaearon/react
that referenced
this pull request
Jun 24, 2019
* Retry failed roots on refresh * Don't prevent retry after error -> render(null) special case The check wasn't very resilient because in Concurrent Mode it looks like we can get further follow-up commits even if we captured an error. So we can't reliably distinguish the case where after an error you _manually_ rendered null. Retrying on an edit after a tree failed _and_ you rendered null in the same tree seems fine. It's also very unlikely a pattern like this actually exists in the wild.
rickhanlonii
pushed a commit
to rickhanlonii/react
that referenced
this pull request
Jun 25, 2019
* Retry failed roots on refresh * Don't prevent retry after error -> render(null) special case The check wasn't very resilient because in Concurrent Mode it looks like we can get further follow-up commits even if we captured an error. So we can't reliably distinguish the case where after an error you _manually_ rendered null. Retrying on an edit after a tree failed _and_ you rendered null in the same tree seems fine. It's also very unlikely a pattern like this actually exists in the wild.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This aligns the behavior for roots with error boundaries. When a root unmounts as a result of an error, if the Fresh runtime is running, it will record its last rendered element in a Map.
There is a new
scheduleRoot(root, element)DevTools hook. Fresh runtime calls it on update for all failed roots with their last rendered elements. This offers us an opportunity to remount the tree.We remove a root from the failed map if there is a further update on it that isn't caused by an error. In order to determine whether an update is caused by an error, we check
effectTag. This is done before callingonCommitFiberRootso that Fresh runtime itself doesn't depend on a particular tag constant.