Add invariant that an object with modified properties must be valid. #2278
Conversation
Release notes: None
| @@ -325,6 +325,11 @@ export default class ObjectValue extends ConcreteValue { | |||
| // never be serialized. | |||
| refuseSerialization: boolean; | |||
|
|
|||
| // Checks whether effects are properly applied. | |||
There was a problem hiding this comment.
I don't see anything that ensures that _isPartial is only true when effects are applied/undone. I thought this was an annotation used in our models/used for snapshotting.
There was a problem hiding this comment.
According to its Flow type definition, _isPartial is AbstractValue | BooleanValue, and it's initialized in the constructor.
The only reason it could ever be undefined is if the effects corresponding to this object have been undone.
|
I'm looking into the bug. |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
I've verified that this invariant is valid with #2402. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
I updated the PR with master and added the check in a bunch of other places. It seems we are hitting more cases now, which is good in some respects. Herman's join PR seems to fix all the failing test cases though. |
NTillmann
changed the title
There was a problem hiding this comment.
NTillmann 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.
Release notes: None
The uncovered bug is tracked in #2279.