Compose without tail duplication #2523
Conversation
There was a problem hiding this comment.
hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
|
There seems to be an issue with our internal bundle when combined with this PR. This invariant is occurring because it's trying to get the "render" method from a React component class that has leaked during the class constructor being called/getDerivedStateFromProps. This should never happen. Out of curiosity, I forced the instance to not leak, thus removing that invariant from possibly occurring and we end up with a broken bundle where only 150 component nodes inline. |
There was a problem hiding this comment.
This seems to break our internal React bundle test in more ways than one (when rebased with master). With master, this issue doesn't occur. I can confirm that changing the logic in composeWithEffects back to the logic on master removes the issue when evaluating the internal bundle.
hermanventer
force-pushed
the
tailDup
branch
from
daeca69
to
68ca7c7
Compare
hermanventer
dismissed
trueadm’s stale review
In the interests of moving forward, I'm adding back the original version of composeEffects and calling it only when the abstractValueImpliesMax flag is > 0. This keeps the internal bundle working and gives the new behavior elsewhere.
There was a problem hiding this comment.
hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release note: none
Closes #2435
Closes #1829
Join.composeWithEffects composes a forked completion with subsequent effects. When two or more forks could end normally, this could result in shallow copies of the subsequent effects. These were then joined together and applied, so it was mostly OK. The generator of the subsequent effects, however, ended up being joined with itself and thus transformed the generator tree to a DAG, which is not desirable for the serializer.
The new approach is to extract a join condition from the forked completion and using it to join the subsequent effects with a newly constructed empty effects. The condition ensures that the subsequent effects are applied only in situations where the forked completion is not abrupt.
Extracting this condition makes for complicated abstract expressions and this uncovered some existing bugs and limitations that are also addressed in this pull request. As a side effect, path conditions are now longer and the time to compile unrolled loops with conditional abrupt completions inside their bodies has gone up so much that the unroll limit had to be lowered.
Please note that the expected output React tests has changed because of re-ordering. I'm none too sure that this re-ordering is necessarily benign, so please review carefully.