Clarify reason for setTextContent helper #11813

jquense · 2017-12-08T19:52:48Z

Some little thing’s I noticed, referencing the code here writing some of react-dom-lite

jquense · 2017-12-08T19:54:19Z

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

-      return;
-    }
-  }
-  node.textContent = text;


i'm not sure this is safe, but it seems to be. We use straight sets on textContent elsewhere

jquense · 2017-12-08T19:58:26Z

also, there seems to be some duplication or extra work being done in diffProperties? I could definitely be reading it wrong, the loops there are hairy, but this loop:

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

Lines 639 to 644 in 8be5a8b

    
           for (styleName in lastStyle) { 
        
             if (lastStyle.hasOwnProperty(styleName)) { 
        
               if (!styleUpdates) { 
        
                 styleUpdates = {}; 
        
               } 
        
               styleUpdates[styleName] = '';

seems to be essentially duplicated lower down in the nextProp loop:

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

Lines 690 to 699 in 8be5a8b

    
             if ( 
        
               lastProp.hasOwnProperty(styleName) && 
        
               (!nextProp || !nextProp.hasOwnProperty(styleName)) 
        
             ) { 
        
               if (!styleUpdates) { 
        
                 styleUpdates = {}; 
        
               } 
        
               styleUpdates[styleName] = ''; 
        
             } 
        
           }

nhunzaker · 2017-12-19T02:26:40Z

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

        }
      } else if (typeof nextProp === 'number') {
-        setTextContent(domElement, '' + nextProp);
+        domElement.textContent = '' + nextProp;


Why stringify here and not line 295?

textContent applies stringify and sanitization AFAIK

nhunzaker · 2017-12-19T02:27:00Z

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

@@ -340,7 +338,7 @@ function updateDOMProperties(
    } else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
      setInnerHTML(domElement, propValue);
    } else if (propKey === CHILDREN) {
-      setTextContent(domElement, propValue);
+      domElement.textContent = propValue;


Likewise, should we stringify?

I think the stringify happens in diffProperties for update, so the value is already a string here

Got it. In any case, this looks consistent with the old behavior. 👍

nhunzaker

This seems good to me, is there anything I can help to verify?

jquense · 2017-12-19T03:04:46Z

I'm not sure how to verify honestly. It's looks to me like the setTextContent was legacy considering the "modern" usages just use text Content, but perhaps someone with more perspective knows? In particular that one branch is really special, I was assuming it was for ie8 (per the comment) but I'm not sure the acceptance criteria for that

nhunzaker · 2017-12-19T03:11:21Z

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

@@ -670,7 +668,7 @@ export function diffProperties(
  }
  for (propKey in nextProps) {
    const nextProp = nextProps[propKey];
-    const lastProp = lastProps != null ? lastProps[propKey] : undefined;
+    const lastProp = lastProps[propKey];


Just thinking about moving carefully, is this change related to the text content change? Could we send this out separately?

I know that's tip toeing quite a bit. Just trying to figure out how to move forward with confidence :)

Good eye :P yeah it's not related to the one change, just another thing I noticed. I did split it out by commit but we can do separate PR's

No harm in being intentional about the changes!

syranide · 2017-12-20T11:14:06Z

@jquense setTextContent should be IE8 only.

syranide · 2017-12-20T11:15:56Z

@jquense EDIT: Specifically because innerText in IE8 behaves unacceptably and actually messes with the text being set.

syranide · 2017-12-20T11:16:51Z

@jquense Sorry for the spam, but here's my origin PR #1864 :)

jquense · 2017-12-20T14:48:30Z

Awesome, thanks for the context @syranide

Anyone else wanna jump in? otherwise good to go?

gaearon · 2017-12-20T15:01:00Z

I don't think this code is currently IE8-motivated. The IE8 part was removed in #11331.

The current code using nodeValue was performance-motivated. @trueadm added it in #7005. If we can run benchmarks and confirm it's no longer causing a difference in modern browsers I think we can remove it, but sounds like we should at least verify that.

jquense · 2017-12-20T15:16:21Z

that's really interesting. The idea is that if the content is a single text node it's faster to set nodeValue there? I'd be happy try and verify, at the very least I'll update the comment :)

trueadm · 2017-12-20T16:31:32Z

It's still much faster to update a single text node via nodeValue than textContent for updates. textContent is faster on initial, as it creates a child TextNode for you. Using textContent for updates is expensive, as it creates a new TextNode and replaces the existing one, plus it has to deal with cases such as null (which removes all child nodes).

jquense · 2017-12-20T18:18:09Z

TIL :)

Thanks for the clarification (and sorry i didn't find the previous work on this before updating!)

syranide · 2017-12-20T18:19:01Z

@trueadm Just curious, could it possibly be faster with .data even? Considering that's a property of the Text-node itself rather than the Element.

update the comment explaining the reason for the helper

trueadm · 2017-12-20T18:34:55Z

@syranide I tried with data but it wasn't as fast as nodeValue. Might be worth benchmarking that again though – I've not tried data for a while. Furthermore, you can pass a string or number to both textContent and nodeValue. They both sanitize to string anyway.

nhunzaker · 2017-12-20T18:47:14Z

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

@@ -736,7 +737,7 @@ const DOMRenderer = ReactFiberReconciler({
    },

    resetTextContent(domElement: Instance): void {
-      domElement.textContent = '';
+      setTextContent(domElement, '');


Semi related:
#11609

commitTextUpdate throws in some cases in IE9. I wonder if using this method there would fix it.

trueadm · 2017-12-22T15:23:32Z

I can also confirm innerText is faster than textContent for creating the initial text and also for updates (in all browsers). Be careful when benchmarking on Chrome, it caches the value and does an auto diff for textContent so you'll need to change the string each time for benchmarking purposes.

syranide · 2017-12-23T01:12:01Z

@trueadm Just for posterity, you should absolutely not use innerText instead of textContent because they are not identical.

trueadm · 2017-12-23T09:58:58Z

@syranide I’m well aware. http://perfectionkills.com/the-poor-misunderstood-innerText/.

* Update comment on setTextContent update the comment explaining the reason for the helper * Use `setTextContent` in ReactDOM for consistency

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

jquense commented Dec 8, 2017

View reviewed changes

nhunzaker reviewed Dec 19, 2017

View reviewed changes

nhunzaker approved these changes Dec 19, 2017

View reviewed changes

nhunzaker reviewed Dec 19, 2017

View reviewed changes

jquense force-pushed the prune branch from 8be5a8b to 4b24e56 Compare December 19, 2017 16:51

jquense changed the title ~~Minor code pruning~~ Remove setTextContent helper Dec 19, 2017

jquense added 2 commits December 20, 2017 13:29

Update comment on setTextContent

a967cd5

update the comment explaining the reason for the helper

Use setTextContent in ReactDOM for consistency

10e8637

jquense force-pushed the prune branch from 4b24e56 to 10e8637 Compare December 20, 2017 18:37

jquense changed the title ~~Remove setTextContent helper~~ Clarify reason for setTextContent helper Dec 20, 2017

nhunzaker reviewed Dec 20, 2017

View reviewed changes

gaearon merged commit 4e044f5 into master Jan 5, 2018

gaearon deleted the prune branch January 5, 2018 18:10

This was referenced Dec 18, 2020

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#38

Open

This was referenced Sep 16, 2021

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify reason for setTextContent helper #11813

Clarify reason for setTextContent helper #11813

jquense commented Dec 8, 2017

jquense Dec 8, 2017

jquense commented Dec 8, 2017 •

edited

Loading

nhunzaker Dec 19, 2017

trueadm Dec 20, 2017

nhunzaker Dec 19, 2017

jquense Dec 19, 2017 •

edited

Loading

nhunzaker Dec 19, 2017

nhunzaker left a comment

jquense commented Dec 19, 2017

nhunzaker Dec 19, 2017

jquense Dec 19, 2017

syranide commented Dec 20, 2017

syranide commented Dec 20, 2017

syranide commented Dec 20, 2017

jquense commented Dec 20, 2017

gaearon commented Dec 20, 2017

jquense commented Dec 20, 2017

trueadm commented Dec 20, 2017 •

edited

Loading

jquense commented Dec 20, 2017

syranide commented Dec 20, 2017 •

edited

Loading

trueadm commented Dec 20, 2017 •

edited

Loading

nhunzaker Dec 20, 2017

trueadm commented Dec 22, 2017 •

edited

Loading

syranide commented Dec 23, 2017

trueadm commented Dec 23, 2017

Clarify reason for setTextContent helper #11813

Clarify reason for setTextContent helper #11813

Conversation

jquense commented Dec 8, 2017

Choose a reason for hiding this comment

jquense commented Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

jquense commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide commented Dec 20, 2017

syranide commented Dec 20, 2017

syranide commented Dec 20, 2017

jquense commented Dec 20, 2017

gaearon commented Dec 20, 2017

jquense commented Dec 20, 2017

trueadm commented Dec 20, 2017 • edited Loading

jquense commented Dec 20, 2017

syranide commented Dec 20, 2017 • edited Loading

trueadm commented Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

trueadm commented Dec 22, 2017 • edited Loading

syranide commented Dec 23, 2017

trueadm commented Dec 23, 2017

jquense commented Dec 8, 2017 •

edited

Loading

jquense Dec 19, 2017 •

edited

Loading

trueadm commented Dec 20, 2017 •

edited

Loading

syranide commented Dec 20, 2017 •

edited

Loading

trueadm commented Dec 20, 2017 •

edited

Loading

trueadm commented Dec 22, 2017 •

edited

Loading