Adds support for abstract length arrays in React reconcilation and serialization #2571
Conversation
| @@ -2047,6 +2049,10 @@ export class ResidualHeapSerializer { | |||
| } | |||
|
|
|||
| _serializeEmptyValue(): BabelNodeExpression { | |||
| if (this.emptySerializesToEmptyString) { | |||
| invariant(this.realm.react.enabled, "emptySerializesToEmptyString is a React optimization"); | |||
| return t.stringLiteral(""); | |||
There was a problem hiding this comment.
Rather than output the empty variable, instead we generate an empty string as the React serialization expects this for cases where the value is a string.
There was a problem hiding this comment.
The special "empty value" is used as a marker value as part of comparisons. It should never leak through as the actual value of anything that's exposed. For that reason, I don't understand how this can fix anything?
Also, the identity if that "empty value" matters. Replacing it in some contexts with something else (that isn't even unique), seems highly dubious to me. Why is this correct?
There was a problem hiding this comment.
If you remove the string literal optimization, you will see __empty leaked out to the output. I was under the assumption that this could happen? If not, I can easily make a repro as a separate issue and remove the above optimization.
There was a problem hiding this comment.
I've removed this form the PR, but now the tests fail as __empty is no longer an empty string but an empty object which gets serialized out and is invalid as a return value for a React render.
There was a problem hiding this comment.
I have a different approach that removes holes from the arrays in the reconciler so we shouldn't hit the same issue but the still overall problem with __empty exists, see the issue above.
src/react/ReactEquivalenceSet.js
Outdated
| result = this.getEquivalentPropertyValue(array, "" + i, currentMap, visitedValues); | ||
| currentMap = result.map; | ||
| if (lengthValue instanceof AbstractValue) { | ||
| currentMap = this.getKey("length", currentMap, visitedValues); |
There was a problem hiding this comment.
Comparing to what happens below for the concrete case, I don't understand how just using "length" once as the key here is supposed to work?
There was a problem hiding this comment.
I should also add the length for the concrete case too.
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
This PR fixes issues with the React hoisting and equivalence system mechanics and serialization where previous, there was no support for abstract length arrays. This includes an optimization for when the React serializer outputs ReactElement children that are conditionals with one side being an empty value (in this case, we can use an empty string instead).
Tests attached that focus on the areas covered in this PR.