Add basic support for throws in React #2502
Conversation
There was a problem hiding this comment.
Rather than intercept EXCEPTION_THROWN, it would be better to instead not throw it in cases where the case was an explicit error rather than that of a JS error. I'll push some changes to help push this along as I'm interested in getting throws supported in both internal bundles if possible.
| @@ -814,8 +813,6 @@ export function getValueFromFunctionCall( | |||
| } else { | |||
| throw error; | |||
| } | |||
| } finally { | |||
| invariant(createdObjects === realm.createdObjects, "realm.createdObjects was not correctly restored"); | |||
There was a problem hiding this comment.
Let's keep this invariant, it's been very useful in finding early evaluation errors. Rather, we should capture the effects of the function call (using evaluateForEffects) and apply them if the createdObjects has been correctly restored. We ran into countless issues in the past that this guarded against, so I don't think we should regress on this if possible.
There was a problem hiding this comment.
Actually, to make this PR easier I'm happy to remove the invariant for now. I can revisit with some changes later this week that I had planned.
|
@calebmer I pushed some changes that I proposed above and also fixed up the React tests. I added an invariant that we run into on the internal React bundle that we'll need to fix in a follow up PR too. If you could review and let me know if you like the changes? |
|
I like your changes @trueadm 👍 Fixed the serializer tests now that |
There was a problem hiding this comment.
trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Starts adding basic support for throws in React. Concretely there are three things this PR does outside of adding tests: 1. Allowing throw side-effects. 2. Removing an invalid invariant. `createdObjects` changes after calling `realm.captureEffects()` and this is expected. Later code which joins/incorporates effects will merge in the captured `createdObjects`. 3. Don’t catch `AbruptCompletion`s and handle them as errors. Instead let them propagate up to the nearest `realm.evaluateForEffects()`. (Or similar function.) I have not run this against the internal web bundle yet. Against the internal React Native bundle we get pretty far without removing throws with these changes. Pull Request resolved: facebookarchive#2502 Reviewed By: trueadm Differential Revision: D9566580 Pulled By: calebmer fbshipit-source-id: 3716a6afd5fc3ae824182ee50e38e51d72126dc2
Starts adding basic support for throws in React. Concretely there are three things this PR does outside of adding tests:
createdObjectschanges after callingrealm.captureEffects()and this is expected. Later code which joins/incorporates effects will merge in the capturedcreatedObjects.AbruptCompletions and handle them as errors. Instead let them propagate up to the nearestrealm.evaluateForEffects(). (Or similar function.)I have not run this against the internal web bundle yet. Against the internal React Native bundle we get pretty far without removing throws with these changes.