Refactor of ReactElement React Props equivalence logic #2138

trueadm · 2018-06-20T13:05:34Z

Release notes: none

After studying the issues in #2137, I understood that we currently make some assumptions in terms of the ReactElement equivalence system that are incompatible with what we want from Prepack going forward. This PR aims at fixing those assumptions so they fall more into line with the changes #2137.

This PR makes the following changes:

All React equivalence is now handled explicitly in the ReactSet/ReactElementSet/ReactPropsSet to ensure all the logic is located together as one and not sprinkled around the codebase.
We use hardModifyReactObjectPropertyBinding to modify final objects that React controls rather than Properties.Set. This function bypasses the normal route of putting the property changes on the effects – thus making it so the property changes are permanent and cannot be reverted. Given the object is immutable, there should never be a case where the property may change anyway – we only change the property as part of the equivalence phase and during reconciliation where the React internals are at play. This is only safe for objects explicitly created by React and marked as final by React – no other object can go through this codepath. Any attempt to use this dangerous function on a non-final object will result in an invariant.
Various bug fixes were fixed that were found when Make modifiedProperties map somewhat immutable #2137 was merged and existing tests failed.
Removed all the logic around makeNotFinal and use hardModifyReactObjectPropertyBinding instead.

…vocing

gaearon · 2018-06-20T13:40:22Z

src/react/ReactElementSet.js

-  constructor(realm: Realm, equivalenceSet: HashSet<AbstractValue>) {
-    this.realm = realm;
-    this.equivalenceSet = equivalenceSet;
+export class ReactElementSet extends ReactSet {


Does inheritance really help here?

I know we use it for the value taxonomy but I'd prefer to avoid it for the rest of the logic. I imagine you could instantiate ReactSet instead, keep it as a field, and delegate to it when necessary.

What's wrong with inheritance? I find this code cleaner and easier to understand/read now.

Same reason we don't use it in components? :-)

Inheritance makes the dependencies between parts very implicit. It's hard for me to tell what is the contract between ReactSet and its subclasses. Do they call its methods? Does it call their methods? Does both happen? Since all interaction happens through this.something() I have to check every method name to verify where it's defined.

This problem usually becomes even more difficult with time, as superclass become coupled to subclasses, and then one of them needs to behave differently but can't "break out".

Using delegation would make it clearer what ReactSet is doing, and how each of the specific "sets" uses this functionality.

I have to disagree here, This isn't the same as UI components, this is business logic. I actually find inheritance is fine in cases like this when there's only a single level of inheritance and the contract is explicit.

I'll refactor the classes again though as I don't want to block this PR on arguments of the validity of inheritance :D

It's not blocking, I'm just finding it harder to understand what the code is doing because of the implicitness.

I've redone it all and removed the classes, it's all functions and argument passing now. Functional FTW

gaearon · 2018-06-20T14:24:22Z

src/react/ReactPropsSet.js

+
+    if (temporalAlias !== undefined) {
+      currentMap = this._getKey("temporalAlias", currentMap, visitedValues);
+      result = this._getValue(temporalAlias, currentMap, visitedValues);


What case does this handle?

I think this would break if there was a prop called temporalAlias (very unlikely but should be easy to fix).

It's for snapshotting, I've addressed in latest commit :)

Can we use a symbol or an object as a key in this case? String clashes with user supplied props (which also use strings).

gaearon

Overall LGTM.

I found it a bit difficult to understand what methods on ReactSet are "internal" to it and which are meant to be called by subclasses. Would prefer delegation but don't care strongly.

Found a few bugs (#2139) while testing this but they're on master too.

gaearon · 2018-06-20T15:04:10Z

Btw I didn't mean to drop classes :-)

I meant that instead of having ReactPropsSet extend ReactSet, you can delegate to it.

class ReactPropsSet {
  reactSet = new ReactSet(whatItActuallyNeeds);

  add() {
    var something = this.reactSet.doSomething();
    // ...
}

Then interaction between them is explicit and I don't need to guess which methods are private to the base class vs which are meant to be accessible by superclass, and whether base class ever calls the superclass.

Anyway, no need to rewrite—hope this explains what I meant.

gaearon · 2018-06-20T15:06:40Z

Let's just revert the last commit? The argument count in functions is wild.

I agree classes are better in this case. I'm sorry if my last comments were confusing. I was strictly speaking about dropping inheritance, not dropping classes.

gaearon · 2018-06-20T15:08:05Z

Anyway. Whatever you think is clearer 👍 Let's not get stuck on this.

This reverts commit bd1ea2a.

hermanventer

Looks good to me.

hermanventer · 2018-06-20T17:22:38Z

src/react/utils.js

@@ -91,6 +91,20 @@ export function isReactElement(val: Value): boolean {
  return false;
 }

+export function isReactProps(val: Value): boolean {


Perhaps call this "inReactProps"? Come to think of it, reactProps is not a very helpful name for someone unfamiliar with the code base.

Maybe isReactPropsObject? A single object is called "props", it's not a plural in this sense.

The fact that it checks a set is an implementation detail. It's meant to be more of a brand check conceptually. Like isArray rather than inArrays or isArrays.

facebook-github-bot

@hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gaearon · 2018-06-20T18:09:14Z

src/react/utils.js

@@ -1153,7 +1153,18 @@ export function hardModifyReactObjectPropertyBinding(
    "hardModifyReactObjectPropertyBinding can only be used on final objects!"
  );
  let binding = object.properties.get(propName);
-  invariant(binding !== undefined);
+  if (binding === undefined) {
+    binding = {


This looks like we're trying to create an object to match the shape below. Can we instead make peace with it being undefined, and remove the invariant below? Object.assign should work either way.

If it's undefined, then we need to create the binding. That's why we do it here.

facebook-github-bot

@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Release notes: none This PR contains some fixes and small tidy ups that would have been in #2138. There was a bug to do with the React props equivalence system and snapshots that was found when #2137 was merged (along with the fix I commented to on that PR). This PR unblocks #2137 to land as it should now pass our internal bundle test. Closes #2146 Differential Revision: D8556958 Pulled By: trueadm fbshipit-source-id: dd774de76059e00af1dff305dd3618c3bfed06ea

trueadm added 4 commits June 20, 2018 12:34

Refactor of ReactElement React Props equivalence logic

9793ac1

better handle temporalAlias values

6d1896c

adds hardModifyReactObjectPropertyBinding and removes hacks around ha…

aa204d3

…vocing

Ensure we do not modify an object unless it is final

8fa5bea

trueadm requested a review from gaearon June 20, 2018 13:05

facebook-github-bot added the CLA Signed label Jun 20, 2018

trueadm mentioned this pull request Jun 20, 2018

Make modifiedProperties map somewhat immutable #2137

Closed

gaearon reviewed Jun 20, 2018

View reviewed changes

gaearon approved these changes Jun 20, 2018

View reviewed changes

ES2015 classes suck, functional FTW :)

bd1ea2a

trueadm added 3 commits June 20, 2018 16:13

Revert "ES2015 classes suck, functional FTW :)"

58ce455

This reverts commit bd1ea2a.

Applied changes as per feedback

c470aeb

Fixes more bugs found related to 2137

1343438

hermanventer approved these changes Jun 20, 2018

View reviewed changes

facebook-github-bot reviewed Jun 20, 2018

View reviewed changes

handle cloneElement bugs from other PR

6f68bbc

gaearon reviewed Jun 20, 2018

View reviewed changes

facebook-github-bot reviewed Jun 20, 2018

View reviewed changes

facebook-github-bot closed this in 0cdafba Jun 20, 2018

trueadm mentioned this pull request Jun 21, 2018

Follow up fixes for 2138 #2146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of ReactElement React Props equivalence logic #2138

Refactor of ReactElement React Props equivalence logic #2138

trueadm commented Jun 20, 2018 •

edited

Loading

gaearon Jun 20, 2018

trueadm Jun 20, 2018 •

edited

Loading

gaearon Jun 20, 2018

trueadm Jun 20, 2018

gaearon Jun 20, 2018

trueadm Jun 20, 2018

gaearon Jun 20, 2018

trueadm Jun 20, 2018

gaearon Jun 20, 2018

gaearon left a comment •

edited

Loading

gaearon commented Jun 20, 2018 •

edited

Loading

gaearon commented Jun 20, 2018

gaearon commented Jun 20, 2018

hermanventer left a comment

hermanventer Jun 20, 2018

gaearon Jun 20, 2018 •

edited

Loading

facebook-github-bot left a comment

gaearon Jun 20, 2018

trueadm Jun 20, 2018

facebook-github-bot left a comment

Refactor of ReactElement React Props equivalence logic #2138

Refactor of ReactElement React Props equivalence logic #2138

Conversation

trueadm commented Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

trueadm Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment • edited Loading

Choose a reason for hiding this comment

gaearon commented Jun 20, 2018 • edited Loading

gaearon commented Jun 20, 2018

gaearon commented Jun 20, 2018

hermanventer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

trueadm commented Jun 20, 2018 •

edited

Loading

trueadm Jun 20, 2018 •

edited

Loading

gaearon left a comment •

edited

Loading

gaearon commented Jun 20, 2018 •

edited

Loading

gaearon Jun 20, 2018 •

edited

Loading