Record current values in the bindings map #2099
Conversation
hermanventer
force-pushed
the
immutableEffects
branch
from
52f189a
to
86237a3
Compare
|
@hermanventer I just checked this out and the failing React test seems to be a failing Jest snapshot. The snapshot looks to have been recording something that should have been failing, so this PR actually fixes a previous regression! If you update the snapshot on your PR, the test will pass ( |
hermanventer
force-pushed
the
immutableEffects
branch
from
86237a3
to
7c40c62
Compare
hermanventer
changed the title
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.
hermanventer
force-pushed
the
immutableEffects
branch
from
7c40c62
to
97735d7
Compare
|
Updated this to no longer require recordModifiedBinding to update the map entry on every modification. This makes the code simpler and also will be consistent with the (only reasonable) way that property bindings will be tracked. |
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
Effects objects are currently stateful: If you apply the effects it becomes an undo map and if you undo the effects, it becomes a redo map.
In most situations this is just fine since apply and undo are usually nicely paired. In some situations, however, we end up applying or undoing the same effects object twice in a row. This causes issues that are extremely hard to debug. I've tried to purge the code base of all such cases, but they just keep coming up. In essence, I am trying to maintain a very subtle invariant that is quite hard to check and I am failing.
I now give up on this invariant and am making Effects into an object that is immutable except when being constructed incrementally. This is the first step towards that.