Refactor shouldSetTextContent & Add tests (#11789) #11792

magicmark · 2017-12-07T06:23:12Z

This PR adds 2 tests to further clarify the issue raised in #11789

Only the test in ReactDOMTextComponent-test.js fails.

It seems like either both tests should pass, or both tests should fail, depending on the desired behavior of passing an object with a toString method to dangerouslySetInnerHTML?

Thanks!

Edit: I've refactored shouldSetTextContent in order to clarify the logic flow, and to add the fix. Let me know if this looks ok. Thanks!

@gaearon

magicmark · 2017-12-08T21:37:36Z

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

+    }
+
+    if (typeof props.dangerouslySetInnerHTML === 'object' && props.dangerouslySetInnerHTML !== null) {
+      if (typeof props.dangerouslySetInnerHTML.__html === 'string') {


Should number be allowed?

gaearon · 2018-01-05T18:09:07Z

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

+        return false;
+      } else if (
+        typeof props.dangerouslySetInnerHTML.__html === 'object' &&
+        typeof props.dangerouslySetInnerHTML.__html.toString === 'function'


I don't understand how this could work. Any JS object has toString defined.

Object.create(null) maybe ?

Ah...

> typeof ({}).toString 'function'

@gaearon good catch, could refactor to use .hasOwnProperty? :)

> ({}).hasOwnProperty('toString') false

It could be inherited from something else. I think in general trying to detect a custom toString is a flawed approach.

Hmm yeah, so maybe back to square 1? Just check for an object, with a check for the existence of toString for corner cases like Object.create(null)?

gaearon · 2018-01-05T19:34:05Z

I'm sorry but I'm spacing out a bit. Can you explain me what the problem is in the existing code, and what is your high-level approach to fixing it?

magicmark · 2018-01-05T20:50:41Z

The issue is outlined in detail here: #11789

but tl;dr: open the console and load this repro: https://codesandbox.io/s/vqkq34o965

With the following:

const HelloObj = {
  toString: () => "Bonjour"
};

const MyComponent = () => (
  <p
    dangerouslySetInnerHTML={{
      __html: HelloObj
    }}
  />
);

hydrate(<MyComponent />, document.getElementById("app"));

When doing SSR, we get a console mismatch warning, and the component turns blank.

This is noteworthy because if we just use render(), the markup is correctly generated. It's only when we use hydrate() on the client that we get errors.

I did some digging into why this occurs, and if my understanding is correct, it is because shouldSetTextContent fails to return true for an object with a toString method passed to __html. Changing shouldSetTextContent to accept this appears to fix this issue.

gaearon · 2018-01-05T20:57:18Z

What is React 15 behavior for this?

magicmark · 2018-01-05T20:59:26Z

Appears to work as expected with React 15, using render() in the client: https://codesandbox.io/s/9zkxjxoqly

(no console errors and the component renders correctly)

gaearon · 2018-01-05T21:03:29Z

It's not exactly equivalent since it's not doing hydration. (React 15 wanted a special attribute and 1:1 markup match for that.)

Although 15 didn't touch the DOM at all during hydration so maybe it doesn't matter. Can you try creating a full hydrating example with 15, still?

gaearon · 2018-01-05T21:04:26Z

Just check for an object, with a check for the existence of toString for corner cases like Object.create(null)?

What did React 15 do for Object.create(null)?

magicmark · 2018-01-05T21:26:35Z

Sorry, updated to use the markup generated from renderToString. Still appears to work as expected. (https://codesandbox.io/s/9zkxjxoqly)

What did React 15 do for Object.create(null)?

renderToString will throw a TypeError (https://codesandbox.io/s/k9y0328475)
render will also throw a TypeError (https://codesandbox.io/s/4wkv48ww60)

gaearon · 2018-01-05T21:29:15Z

I'm okay with a fix that keeps React 15 behavior then.

pull-bot · 2018-04-26T20:31:51Z

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: 7c39328...0ab6817

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	611.77 KB	612.17 KB	141.34 KB	141.43 KB	UMD_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.1%	100.4 KB	100.5 KB	31.88 KB	31.91 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	596.15 KB	596.55 KB	137.21 KB	137.29 KB	NODE_DEV
react-dom.production.min.js	🔺+0.1%	🔺+0.1%	98.84 KB	98.94 KB	31.09 KB	31.11 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	620.58 KB	620.99 KB	139.91 KB	140.01 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.1%	0.0%	284.42 KB	284.57 KB	51.96 KB	51.99 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

magicmark · 2018-04-26T20:49:19Z

Hi @gaearon - I've changed the logic slightly to clarify the intended behavior - in that we no longer explicitly check for toString, and deliberately allow any type of Object through.

Tests pass, and I built locally and verified that I still see the TypeError stack traces for Object.create(null).

(fwiw, when I built locally with my old diff, I still saw the same TypeError, but this diff clarifies the intended behavior)

Can you think of anything else I might be missing here, or any other tests I could add? Lot of moving parts here 😌

Thanks!

gaearon

Need to fix the null comparison, add more tests, and a few other nits.

gaearon · 2018-06-20T15:39:17Z

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

+      // the `toString` method of objects. (#11792)
+      if (
+        typeof props.dangerouslySetInnerHTML.__html === 'object' &&
+        props.dangerouslySetInnerHTML.__html !== 'null'


Why are we comparing to a string literal here? Is this a bug? In that case it means we're missing a test that would have caught it.

gaearon · 2018-06-20T15:39:46Z

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

+      return true;
+    }
+
+    if (


Overall: we are reading props.dangerouslySetInnerHTML a lot of times here. I know old code did too, but this makes reading a bit difficult in all further conditions. Mind extracting it to a variable?

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

packages/react-dom/src/__tests__/ReactDOMTextComponent-test.js

@@ -197,6 +197,16 @@ describe('ReactDOMTextComponent', () => {
    expect(el.textContent).toBe('');
  });

+  it('can reconcile text from pre-rendered markup using dangerouslySetInnerHTML and an object with toString', () => {


Can we add some tests to ReactDOMServerIntegration* suite instead? They will test all possible combinations and verify those match between client-only/server-only/hydration scenarios.

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

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

+      ) {
+        return true;
+      }
+    }


Is it strings and objects specifically that are allowed? What if __html is a number, for example?

What did React 15 do in this case? Can you point to similar logic there so we can compare the behavior?

gaearon · 2018-08-09T15:20:26Z

This stalled so I'm sending a different PR: #13353
Thanks for getting it started!

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

magicmark force-pushed the master branch 2 times, most recently from 98d59fd to c75f47f Compare December 7, 2017 06:31

magicmark changed the title ~~Add failing test to highlight issue 11789~~ Refactor shouldSetTextContent to fix 11789 Dec 8, 2017

magicmark force-pushed the master branch from 1fd2265 to af28c5f Compare December 8, 2017 21:35

magicmark changed the title ~~Refactor shouldSetTextContent to fix 11789~~ Refactor shouldSetTextContent to fix 11789 & Add tests Dec 8, 2017

magicmark commented Dec 8, 2017

View reviewed changes

magicmark changed the title ~~Refactor shouldSetTextContent to fix 11789 & Add tests~~ Refactor shouldSetTextContent & Add tests (#11789) Dec 8, 2017

magicmark force-pushed the master branch from af28c5f to 257d90f Compare December 8, 2017 21:47

gaearon reviewed Jan 5, 2018

View reviewed changes

sneridagh mentioned this pull request Apr 12, 2018

Hydrating a component with dangerouslySetInnerHTML coming from SSR fails and shows empty plone/volto#137

Closed

Refactor shouldSetTextContent & add tests (facebook#11789)

0ab6817

magicmark force-pushed the master branch from 257d90f to 0ab6817 Compare April 26, 2018 20:21

magicmark mentioned this pull request May 9, 2018

hydrating a component with dangerouslySetInnerHTML and toString causes a warning, and the component to not render #11789

Closed

gaearon requested changes Jun 20, 2018

View reviewed changes

gaearon closed this Aug 9, 2018

Refactor shouldSetTextContent & Add tests (#11789) #11792

Refactor shouldSetTextContent & Add tests (#11789) #11792

Uh oh!

Conversation

magicmark commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jquense Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicmark Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Jan 5, 2018

Uh oh!

magicmark commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Jan 5, 2018

Uh oh!

magicmark commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Jan 5, 2018

Uh oh!

gaearon commented Jan 5, 2018

Uh oh!

magicmark commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Jan 5, 2018

Uh oh!

pull-bot commented Apr 26, 2018

react-dom

Uh oh!

magicmark commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Aug 9, 2018

Uh oh!

Uh oh!

magicmark commented Dec 7, 2017 •

edited

Loading

jquense Jan 5, 2018 •

edited

Loading

magicmark Jan 5, 2018 •

edited

Loading

magicmark commented Jan 5, 2018 •

edited

Loading

magicmark commented Jan 5, 2018 •

edited

Loading

magicmark commented Jan 5, 2018 •

edited

Loading

magicmark commented Apr 26, 2018 •

edited

Loading