Inline DOM property configs #10805

gaearon · 2017-09-24T20:55:12Z

This replaces DOM injections with static switch cases.
I think this is a lot simpler to follow.

See individual commits. I took the strategy of getting rid of individual properties on the injected configs, then getting rid of getPropertyInfo() branching calls, and finally removing the configs.

gaearon · 2017-09-25T14:17:09Z

Okay, the initial version is ready.
Curiously, despite the switches, it still reduces the bundle size a bit (2%).

Still need to test it with attribute table, add Flow types, and maybe simplify the callers.
We might also be making more function calls so worth testing if the perf is the same.

But curious to hear what you all think about this direction.

trueadm · 2017-09-25T15:39:34Z

I wonder how this will impact runtime performance as object lookups tend to be quite a bit faster than large switch statements – although things might be different as I've not looked into this for a while. Considering this is a hot path though, we should look into it more. I can help with this if needed :)

I know other virtual DOM libraries recently started using ES2015 Sets for attribute/property lookups as they were giving great performance results compared to object lookups and switches.

gaearon · 2017-09-25T16:56:35Z

The switches are not the point and I can change them to lookups (would be nice to confirm this with data). This is more about removing the injection and consolidating the lookups in one place.

aweary

I like the approach of inlining over injection. Currently, there's an error with how this changes the behavior for unknown attributes, which I left a comment about.

Here's a small test case showing the issue that this introduces:

http://dom-config-inline-attribute-error.surge.sh/

aweary · 2017-09-25T17:25:59Z

src/renderers/dom/shared/DOMProperty.js

+        return setDOMValueAttribute;
+      default:
+        return null;
+    }


Should we use a switch case here if there's only two possible return values? We could shave a off few more bytes with a ternary or if/else

I don't really care about this yet, as we'll likely change what's inside of these methods anyway. I'm mostly looking for feedback on the overall changes in how functions call each other.

aweary · 2017-09-25T17:27:46Z

src/renderers/dom/shared/DOMProperty.js

+      // Removed when strictly equal to false; present without a value when
+      // strictly equal to true; present with a value otherwise.
+      case 'download':
+        return 'overloadedBoolean';


I wonder if it would minify better if we used constants instead of string literals?

Yes, flowifying and turning into something simper is in the plan. (Although it's already smaller than the original.)

AFAIK v8 used to/does deop switches without string literals as the case, I couldn't find anything recent that gave clarity on if that was still true, it may not matter here either. (I mention it because the return here is used in a switch elsewhere yeah?)

aweary · 2017-09-25T17:44:05Z

src/renderers/dom/shared/DOMProperty.js

   * Checks whether a property name is a writeable attribute.
   * @method
   */
  shouldSetAttribute: function(name, value) {
-    if (DOMProperty.isReservedProp(name)) {
+    if (name !== 'style' && DOMProperty.isReservedProp(name)) {


Why is style being special-cased here now?

It's the only reserved attribute that actually needs to appear in the DOM. Previously this was special cased by leaving it in the DOM config (even though the flag was set to 0 with style: 0). Now we don't have a config so it has to go somewhere else.

But in what case will shouldSetAttribute be called for the style prop? From what I can tell ReactDOMFiberComponent always checks for the style prop early and defers to CSSPropertyOperations. I don't think we're explicitly setting the style attribute anywhere.

Is it the SSR code path? I didn't check that 😀

edit: I checked and, as expected, it isn't called for SSR either @gaearon

aweary · 2017-09-25T18:04:57Z

src/renderers/dom/shared/DOMPropertyOperations.js

-      DOMPropertyOperations.setValueForAttribute(
-        node,
-        name,
-        DOMProperty.shouldSetAttribute(name, value) ? value : null,


Doesn't this change the semantics of how unknown attributes are handled since the propertInfo check is removed? Now unknown attributes aren't being handled by setValueForAttribute so they aren't validated by isAttributeNameSafe, which means that invalid attributes will now cause a fatal error, e.g

Failed to execute 'setAttribute' on 'Element': 'foo$$bar' is not a valid attribute name.

In general I found the distinction between *ForProperty and *ForAttribute very confusing because it's often lying ("properties" can use attributes).

This indeed seems like a bug, can you write a test case for it?

@gaearon I agree it's confusing, I wouldn't think that setValueForProperty would be setting attributes. I pushed a commit to this branch with a failing test.

I'd really like to get rid of this distinction if we can. It always felt a like to much indirection. At the very least, it would be nice if the property and attribute paths either had clearer separation, or we consolidated them and made sure the isAttributeNameSafe validation is applied.

I think as a rule of thumb, if you have an API like setValueForProperty it shouldn't ever hit a code path that sets it as an attribute, and vice-versa. It's confusing that there's code like node[name] = value inside shouldSetAttribute checks. From what I can tell, the naming scheme is confusing two different types of properties: what we just call "props" and properties that we need to be set as a property (what shouldUseProperty checks).

gaearon · 2017-09-25T18:56:35Z

I also noticed that unknown attributes on SVG started respecting capitalization. e.g. <svg><path unknownAttribute="lol" /></svg> is now unknownAttribute, not unknownattribute.

I’m not sure if it’s bad or which change caused it. If you have ideas please share :-)

nhunzaker

Overall, I really like this approach, but I am genuinely curious about the performance differences.

Still, this feels like a big maintainability win.

nhunzaker · 2017-09-25T18:46:47Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+            domElement,
+            propKey,
+            nextProp,
+          );


If anything, I already appreciate the reduction in branching.

nhunzaker · 2017-09-25T18:50:55Z

src/renderers/dom/shared/DOMPropertyOperations.js

-      DOMPropertyOperations.setValueForAttribute(
-        node,
-        name,
-        DOMProperty.shouldSetAttribute(name, value) ? value : null,


I'd really like to get rid of this distinction if we can. It always felt a like to much indirection. At the very least, it would be nice if the property and attribute paths either had clearer separation, or we consolidated them and made sure the isAttributeNameSafe validation is applied.

nhunzaker · 2017-09-25T18:55:34Z

src/renderers/dom/shared/DOMPropertyOperations.js

-        }
+    var mutationMethod = DOMProperty.getMutationMethod(name);
+    if (mutationMethod) {
+      mutationMethod(node, undefined);


Semi-unrelated, but method that this gets called for is value, which has this null guard:

if (value == null) { return node.removeAttribute('value'); }

I'm not sure if there is a big distinction between null and undefined in other parts of the code, but passing null here, like mutationMethod(node, null) feels better to me. What do you think?

Really, I think if we stop syncing the value attribute with the value property, we can just drop mutation methods all together.

We could just not pass the second argument at all since it will default to undefined anyways, but I would prefer null here as well. It's also shorter 😄

Really, I think if we stop syncing the value attribute with the value property, we can just drop mutation methods all together.

I wonder if we could just inline the getMutationMethod calls as well, since there's only one property that uses mutation methods. Like:

if (name === "value") { setDOMValueAttribute(node, null) }

nhunzaker · 2017-09-25T19:05:09Z

also noticed that unknown attributes on SVG started respecting capitalization. e.g. is now unknownAttribute, not unknownattribute.

Personally I think this is an enhancement, on the current 16 RC, if you do <svg unknownAttribute="lol"> does SSR hydration warn about bad casing?

gaearon · 2017-09-25T19:06:24Z

I haven't checked. Want to send a PR to get this check into attribute table?

gaearon · 2017-09-25T19:15:33Z

Regarding <svg unknownAttribute="lol"> I would expect to see this warning:

React does not recognize the `%s` prop on a DOM element.

Not sure why I didn't. Or maybe I missed it.

aweary · 2017-09-25T19:29:52Z

Not sure why I didn't. Or maybe I missed it.

@gaearon I tested<svg unknownAttribute="lol" /> locally and I did get the expected warning.

gaearon · 2018-01-05T16:27:29Z

Meh. I removed some of this indirection in other PRs and the rest isn't really helpful.

Stop exposing DOMProperty to www

cf2e246

facebook-github-bot added the CLA Signed label Sep 24, 2017

gaearon force-pushed the dom-injection branch 2 times, most recently from e506425 to c64b73f Compare September 25, 2017 11:11

gaearon added 6 commits September 25, 2017 13:01

Remove DOM injection for attributeName

1fcd2f3

Remove DOM injection for propertyName

8f6dc26

Remove DOM injection for mutationMethod

cbca8df

Remove DOM injection for MUST_USE_PROPERTY flag

13cc84b

Remove DOM injection for HAS_POSITIVE_NUMERIC_VALUE flag

93f2aa4

Move shouldIgnoreValue into DOMProperty

570e844

gaearon force-pushed the dom-injection branch from 18be54f to 1508ef3 Compare September 25, 2017 12:09

gaearon added 5 commits September 25, 2017 15:08

Remove DOM injection for remaining HAS_*_VALUE flags

2017452

Remove getPropertyInfo() branching in SSR validation

8b47546

Remove getPropertyInfo() branching in SSR markup generation

061fb15

Remove getPropertyInfo() branching in client-side logic

bf858ef

Remove getPropertyInfo() and remaining property configs

82dcc93

gaearon force-pushed the dom-injection branch from 791e3c1 to 82dcc93 Compare September 25, 2017 14:09

gaearon changed the title ~~[WIP] Inline DOM property configs~~ Inline DOM property configs Sep 25, 2017

gaearon requested review from sophiebits, sebmarkbage and nhunzaker September 25, 2017 14:22

aweary suggested changes Sep 25, 2017

View reviewed changes

Add failing test for invalid attribute warning

dea8439

nhunzaker approved these changes Sep 25, 2017

View reviewed changes

gaearon mentioned this pull request Oct 2, 2017

Fix tabIndex attribute for SVG #11033

Merged

This was referenced Oct 11, 2017

Keep autoFocus attribute in the DOM #11192

Merged

[CS] Persistent Updates #11260

Merged

gaearon closed this Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline DOM property configs #10805

Inline DOM property configs #10805

gaearon commented Sep 24, 2017 •

edited

Loading

gaearon commented Sep 25, 2017 •

edited

Loading

trueadm commented Sep 25, 2017 •

edited

Loading

gaearon commented Sep 25, 2017

aweary left a comment •

edited

Loading

aweary Sep 25, 2017

gaearon Sep 25, 2017

aweary Sep 25, 2017

gaearon Sep 25, 2017

jquense Sep 25, 2017

aweary Sep 25, 2017

gaearon Sep 25, 2017 •

edited

Loading

aweary Sep 25, 2017

aweary Sep 25, 2017 •

edited

Loading

aweary Sep 25, 2017

gaearon Sep 25, 2017

aweary Sep 25, 2017

nhunzaker Sep 25, 2017

aweary Sep 25, 2017

gaearon commented Sep 25, 2017

nhunzaker left a comment

nhunzaker Sep 25, 2017

nhunzaker Sep 25, 2017

nhunzaker Sep 25, 2017

nhunzaker Sep 25, 2017

aweary Sep 25, 2017

aweary Sep 25, 2017 •

edited

Loading

nhunzaker commented Sep 25, 2017

gaearon commented Sep 25, 2017

gaearon commented Sep 25, 2017 •

edited

Loading

aweary commented Sep 25, 2017 •

edited

Loading

gaearon commented Jan 5, 2018

Inline DOM property configs #10805

Inline DOM property configs #10805

Conversation

gaearon commented Sep 24, 2017 • edited Loading

gaearon commented Sep 25, 2017 • edited Loading

trueadm commented Sep 25, 2017 • edited Loading

gaearon commented Sep 25, 2017

aweary left a comment • 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

gaearon Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary Sep 25, 2017 • 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

gaearon commented Sep 25, 2017

nhunzaker 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

aweary Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

nhunzaker commented Sep 25, 2017

gaearon commented Sep 25, 2017

gaearon commented Sep 25, 2017 • edited Loading

aweary commented Sep 25, 2017 • edited Loading

gaearon commented Jan 5, 2018

gaearon commented Sep 24, 2017 •

edited

Loading

gaearon commented Sep 25, 2017 •

edited

Loading

trueadm commented Sep 25, 2017 •

edited

Loading

aweary left a comment •

edited

Loading

gaearon Sep 25, 2017 •

edited

Loading

aweary Sep 25, 2017 •

edited

Loading

aweary Sep 25, 2017 •

edited

Loading

gaearon commented Sep 25, 2017 •

edited

Loading

aweary commented Sep 25, 2017 •

edited

Loading