Refactor DOM attribute code #11804

gaearon · 2017-12-08T00:19:32Z

The main goal here is to factor the DOM attribute code in the light of how it currently works, instead of how it was conceived five years ago.

There are quite a few things in the current code that don't make sense anymore. For example, the "property" vs "attribute" distinction in method naming doesn't actually correspond to properties vs attributes. Another example: shouldSetAttribute doesn't actually determine whether to set an attribute. These were all results of incremental changes where we tried to tweak the behavior with the least amount of code. But they ended up making the overall flow confusing and adding a lot of special cases.

This is an attempt to simplify the code without changing what it does. Individual commits tell how I arrived at that. This mostly changes the logic around setting attributes/properties and doesn't touch the other parts that are still structured the old way (such as get*Attribute|Property pairs used for validating SSR). I think we can get to those a bit later.

gaearon · 2017-12-08T00:40:43Z

Lol, initial bench results were seriously skewed because my screensaver turned on 😄

It seems like this is a little bit slower but with huge variance. So hard to say anything conclusive.

trueadm · 2017-12-08T00:52:42Z

packages/react-dom/src/client/DOMPropertyOperations.js

-      }
+  // The rest are treated as attributes with special cases.
+  const {attributeName, attributeNamespace} = propertyInfo;
+  if (value === null) {


Can value be undefined?

No, this would get caught by:

if (shouldTreatAttributeValueAsNull(name, value, isCustomComponentTag)) { value = null; }

trueadm · 2017-12-08T00:54:54Z

packages/react-dom/src/shared/DOMProperty.js

+}
+
+export function shouldTreatAttributeValueAsNull(name, value) {
+  if (value === null || typeof value === 'undefined') {


why do we need to do typeof value === 'undefined'? why not just value === undefined?

Consistency with all other checks. (I don't care either way but IMO we should change them all together if we want to.)

trueadm · 2017-12-08T00:56:13Z

packages/react-dom/src/shared/DOMProperty.js

+  return prefix !== 'data-' && prefix !== 'aria-';
+}
+
+export function shouldTreatAttributeValueAsNull(name, value) {


Can we add type annotations to this function?

trueadm · 2017-12-08T00:56:41Z

packages/react-dom/src/server/DOMMarkupOperations.js

@@ -80,30 +68,29 @@ export function createMarkupForRoot() {
 * @return {?string} Markup string, or null if the property was invalid.
 */
 export function createMarkupForProperty(name, value) {


Can we also added type annotations here whilst we're at it?

trueadm

Overall, it looks good – the slight performance regression isn't a concern to me. Please, as this is a refactor PR, can we add type annotations to the functions used in this process. I think it will add to the readability and help reduce further bugs if we can. Otherwise, awesome stuff!

trueadm · 2017-12-08T00:58:57Z

packages/react-dom/src/client/DOMPropertyOperations.js

@@ -152,84 +127,62 @@ export function getValueForAttribute(node, name, expected) {
 * @param {string} name
 * @param {*} value
 */
-export function setValueForProperty(node, name, value) {
-  const propertyInfo = getPropertyInfo(name);
+export function setValueForProperty(node, name, value, isCustomComponentTag) {


Type annotations here too please?

nhunzaker · 2017-12-08T01:07:08Z

packages/react-dom/src/server/DOMMarkupOperations.js

+  }
+  if (value === null) {
+    return '';
+  }


Could you just return '' inside of the shouldTreatAttributeValueAsNull block? When does shouldTreatAttributeValueAsNull return false but the value is null?

nhunzaker · 2017-12-08T01:12:12Z

packages/react-dom/src/shared/DOMProperty.js

+  return false;
+}
+
+export function isBadlyTypedAttributeValue(name, value) {


nhunzaker · 2017-12-08T01:13:18Z

packages/react-dom/src/shared/DOMProperty.js

+  if (isBadlyTypedAttributeValue(name, value)) {
+    return true;
+  }
+  return false;


Why not just return isBadlyTypedAttributeValue(name, value)?

I noticed some patterns weren't being tested.

The branching before the call is unnecessary because setValueForProperty() already has an internal branch that delegates to deleteValueForProperty() for null and undefined through the shouldIgnoreValue() check. The goal is to start unifying these methods because their separation doesn't reflect the current behavior (e.g. for unknown properties) anymore, and obscures what actually happens with different inputs.

Now we don't read propertyInfo twice in this case. I also dropped a few early returns. I added them a while ago when we had Stack-only tracking of DOM operations, and some operations were being counted twice because of how this code is structured. This isn't a problem anymore (both because we don't track operations, and because I've just inlined this method call).

The special cases for null and undefined already exist in setValueForAttribute().

Their naming is pretty confusing by now. For example setValueForProperty() calls setValueForAttribute() when shouldSetAttribute() is false (!). I want to refactor (as in, inline and then maybe factor it out differently) the relation between them. For now, I'm consolidating the callers to use setValueForProperty().

The naming of these methods is still very vague and conflicting in some cases. Will need further work.

This makes the flow clearer in my opinion.

It was previously duplicated. It's also suspiciously similar in purpose to shouldTreatAttributeValueAsNull() so I want to see if there is a way to unify them.

Its naming was confusing and it was used all over the place instead of more specific checks. Now that we only have one call site, we might as well inline and get rid of it.

aweary · 2017-12-08T18:47:25Z

packages/react-dom/src/client/DOMPropertyOperations.js

+  // If the prop is in the special list, treat it as a simple attribute.
+  if (!propertyInfo) {
+    if (isAttributeNameSafe(name)) {
+      const attributeName = name;


Why alias name to attributeName?

To make it extra clear you're not supposed to call setAttribute with name unless you know what you're doing.

aweary · 2017-12-08T18:56:38Z

packages/react-dom/src/client/DOMPropertyOperations.js

- * @param {DOMElement} node
- * @param {string} name
- */
-export function deleteValueForProperty(node, name) {


Can you rename this test since deleteValueForProperty doesn't exist anymore?

Do we care? There's plenty of internal names, even in test files (e.g. ReactDOMComponent-test). I agree we need to clean it up by functionality rather than file, but don't think it's necessary to do in the scope of this PR.

Using names of internal APIs that exist is different. In this case, it's referrencing a function that no longer exists. If I was looking to contribute and encountered that, I would be confused that the function didn't exist anywhere in the codebase.

It's not critical, but since you remove it feels like it's in scope to fix any lingering references. If we're going to restructure the tests soon anyways, it probably doesn't matter.

I guess what I'm referring to is that there are already plenty examples where internal names don't match tests. It used to be important for them to be in sync when unit test tested internals, but they don't anymore. I agree it's confusing, but IMO mismatches in test file names are even more confusing.

Conceptually those tests are about deleting. I wouldn't move them into set*ForProperty just because those code paths moved there. Instead I would've preferred to rename all tests to human readable descriptions (e.g. "deletion"). But then it's already inconsistent in the current test codebase so I think it's easier to do in one big cleanup later.

aweary · 2017-12-08T18:58:38Z

packages/react-dom/src/client/DOMPropertyOperations.js

+  if (shouldTreatAttributeValueAsNull(name, value, isCustomComponentTag)) {
+    value = null;
+  }
+  // If the prop is in the special list, treat it as a simple attribute.


Did you mean if the prop is not in the special list?

aweary · 2017-12-08T19:12:37Z

packages/react-dom/src/client/DOMPropertyOperations.js

+  if (mustUseProperty) {
+    const {propertyName} = propertyInfo;
+    if (value === null) {
+      (node: any)[propertyName] = hasBooleanValue ? false : '';


All attributes that must use property are boolean values, so technically this check is unnecessary. We could add a new attribute type like HAS_BOOLEAN_PROPERTY to simplify this, and get rid of the bitmasks again (happy to do in a follow up)

Let's follow up. This diff is getting hard to deal with.

trueadm

Thanks for adding Flow type annotations. LGTM (minus lint error I think?) :)

gaearon · 2017-12-08T20:42:41Z

Ran benchmarks a few times, seems neutral.

This reverts commit 47783e8.

facebook-github-bot added the CLA Signed label Dec 8, 2017

gaearon requested review from nhunzaker, trueadm and aweary December 8, 2017 00:20

gaearon force-pushed the dom-injection-2 branch from 31eacd6 to 8669be3 Compare December 8, 2017 00:41

trueadm reviewed Dec 8, 2017

View reviewed changes

trueadm suggested changes Dec 8, 2017

View reviewed changes

trueadm reviewed Dec 8, 2017

View reviewed changes

nhunzaker reviewed Dec 8, 2017

View reviewed changes

gaearon force-pushed the dom-injection-2 branch from 8669be3 to bd30e25 Compare December 8, 2017 15:55

gaearon added 7 commits December 8, 2017 17:21

Harden tests around init/addition/update/removal of aliased attributes

24fa7e5

I noticed some patterns weren't being tested.

Inline deleteValueForAttribute() into setValueForAttribute()

9370492

The special cases for null and undefined already exist in setValueForAttribute().

Delete some dead code

171795b

Make it more obvious where we skip and when we reset attributes

7022000

The naming of these methods is still very vague and conflicting in some cases. Will need further work.

gaearon force-pushed the dom-injection-2 branch 2 times, most recently from 482a300 to 908a807 Compare December 8, 2017 18:37

gaearon added 5 commits December 8, 2017 18:44

Rewrite setValueForProperty() with early exits

4018f7b

This makes the flow clearer in my opinion.

Move shouldIgnoreValue() into DOMProperty

915b1c6

It was previously duplicated. It's also suspiciously similar in purpose to shouldTreatAttributeValueAsNull() so I want to see if there is a way to unify them.

Use more specific methods for testing validity

22646c7

Unify shouldTreatAttributeValueAsNull() and shouldIgnoreValue()

ec83eac

Remove shouldSetAttribute()

2484117

Its naming was confusing and it was used all over the place instead of more specific checks. Now that we only have one call site, we might as well inline and get rid of it.

gaearon force-pushed the dom-injection-2 branch from 908a807 to 2484117 Compare December 8, 2017 18:44

gaearon added 3 commits December 8, 2017 18:50

Remove unnecessary condition

388498e

Remove another unnecessary condition

632dabd

Add Flow coverage

50c7624

aweary reviewed Dec 8, 2017

View reviewed changes

Oops

57d4169

trueadm approved these changes Dec 8, 2017

View reviewed changes

Fix lint (ESLint complains about Flow suppression)

463c4f5

nhunzaker approved these changes Dec 8, 2017

View reviewed changes

gaearon merged commit 47783e8 into facebook:master Dec 8, 2017

gaearon deleted the dom-injection-2 branch December 8, 2017 20:42

gaearon added a commit that referenced this pull request Dec 8, 2017

Revert "Refactor DOM attribute code (#11804)"

aaddf38

This reverts commit 47783e8.

gaearon mentioned this pull request Dec 8, 2017

Revert "Refactor DOM attribute code" #11814

Merged

gaearon added a commit that referenced this pull request Dec 8, 2017

Revert "Refactor DOM attribute code (#11804)" (#11814)

d9869a4

This reverts commit 47783e8.

gaearon mentioned this pull request Dec 8, 2017

Refactor DOM attribute code (take two) #11815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DOM attribute code #11804

Refactor DOM attribute code #11804

gaearon commented Dec 8, 2017 •

edited

gaearon commented Dec 8, 2017

trueadm Dec 8, 2017

gaearon Dec 8, 2017

trueadm Dec 8, 2017 •

edited

gaearon Dec 8, 2017

trueadm Dec 8, 2017

trueadm Dec 8, 2017

trueadm left a comment

trueadm Dec 8, 2017

nhunzaker Dec 8, 2017

gaearon Dec 8, 2017

nhunzaker Dec 8, 2017

nhunzaker Dec 8, 2017

aweary Dec 8, 2017

gaearon Dec 8, 2017

aweary Dec 8, 2017

gaearon Dec 8, 2017

aweary Dec 8, 2017

gaearon Dec 8, 2017

aweary Dec 8, 2017

gaearon Dec 8, 2017

aweary Dec 8, 2017 •

edited

gaearon Dec 8, 2017

trueadm left a comment •

edited

gaearon commented Dec 8, 2017

Refactor DOM attribute code #11804

Refactor DOM attribute code #11804

Conversation

gaearon commented Dec 8, 2017 • edited

gaearon commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm Dec 8, 2017 • edited

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

trueadm left a 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

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

aweary Dec 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm left a comment • edited

Choose a reason for hiding this comment

gaearon commented Dec 8, 2017

gaearon commented Dec 8, 2017 •

edited

trueadm Dec 8, 2017 •

edited

aweary Dec 8, 2017 •

edited

trueadm left a comment •

edited