fix renderToString fails with array type children when react-dom/server render #10221

evilbs · 2017-07-19T17:55:37Z

fix for #10212 , server renderer can support when returning array in render function

…dom/server render

facebook-github-bot · 2017-07-19T17:55:42Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2017-07-19T17:59:02Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

gaearon · 2017-07-19T18:49:18Z

Can you please add an integration test verifying this works? This might be a good place.

evilbs · 2017-07-19T19:50:21Z

yes, I can, I'm going to increase right away

…ildren

evilbs · 2017-07-20T15:58:46Z

hi, @gaearon，

We need to set disableNewFiberFeatures = false when renderToString with array type children.
This is reference code.

But fiber can not return undefined type child when disableNewFiberFeatures = false, There are a integration test return undefined type child, so test verifying failed. There will be a conflict.
checking undefined type child
ReactDOMProduction-test.js

Can you provide your opinion?

gaearon · 2017-07-20T16:11:48Z

We need to set disableNewFiberFeatures = false when renderToString with array type children.

This sounds right.
I think you can set it on this line:

    const ReactFeatureFlags = require('ReactFeatureFlags');
    ReactFeatureFlags.disableNewFiberFeatures = false;

This won’t affect other tests because each test file is isolated.

Here is an example test file that already does this.

evilbs · 2017-07-20T16:25:02Z

thx, Integration test has passed, I immediately resubmit pr.

gaearon · 2017-07-20T16:35:28Z

You can push a new commit to this branch and it will appear in this PR.

gaearon · 2017-07-20T17:04:06Z

Hmm. I think something got messed up with the commit because it reverts recent changes to master. If you'd like, try creating a new PR instead.

gaearon · 2017-07-20T17:04:48Z

Oh wait, no, it seems okay. Just try merging master into it.

git fetch origin
git merge origin/master
git push

evilbs · 2017-07-20T17:07:40Z

Okay, it's done

gaearon · 2017-07-20T17:12:57Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

+        const e = await render(<AppComponent />);
+        const parentNode = e.parentNode;
+        expect(parentNode.childNodes[0].tagName).toBe('DIV');
+        expect(parentNode.childNodes[1].tagName).toBe('DIV');


Let's render two different types? e.g. [<div key="1" />, <p key="2" />].

ok, good idea.

gaearon · 2017-07-20T17:14:43Z

src/renderers/shared/server/ReactPartialRenderer.js

+            footer: '',
+          };
+          if (__DEV__) {
+            frame.debugElementStack = [];


Why do we need to do this? (Note: I'm not very familiar with this code so I'm learning it too.)

"setCurrentDebugStack" method will set debugElementStack.length = 1, This is to init the debugElementStack array. The same method handles when HostCompoment return array type children.https://github.com/facebook/react/blob/master/src/renderers/shared/server/ReactPartialRenderer.js#L769

Do warnings still work? You can add a test similar to this one but using an array.

@gaearo, It's done. Please review.

…er render

gaearon · 2017-07-20T19:28:02Z

src/renderers/dom/ReactDOMStringRenderer.js

@@ -22,7 +22,7 @@ var ReactPartialRenderer = require('ReactPartialRenderer');
 */
 function renderToString(element) {
  invariant(
-    React.isValidElement(element),
+    Array.isArray(element) || React.isValidElement(element),


I'm not sure this is sufficient. For example it could be a nested array. Conversely, it could be an array of invalid things (e.g. array of objects), and we'd want to throw in that case.

I think the validation should be moved to a further stage at the point where we actually process the next frame. Which is the same strategy we do with Fiber. This way we don’t do validation immediately, but we throw if we meet something invalid.

I'm not sure this is sufficient. For example it could be a nested array. Conversely, it could be an array of invalid things (e.g. array of objects), and we'd want to throw in that case.

I think here can use 'React.Children.toArray' method to deal with this situation.

nested array: Will flatten the array.
[[<Hello />,<Dan />],<Cool />] => [<Hello />,<Dan />,<Cool />]

array of objects: Will throw when passed object does not have an iterator.

I think the validation should be moved to a further stage at the point where we actually process the next frame. Which is the same strategy we do with Fiber. This way we don’t do validation immediately, but we throw if we meet something invalid.

if disableNewFiberFeatures=false then don't do validation immediately.

gaearon · 2017-07-20T19:28:34Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

@@ -333,16 +330,18 @@ describe('ReactDOMServerIntegration', () => {
    });

    if (ReactDOMFeatureFlags.useFiber) {
-      itRenders('a array type children as a child', async render => {
-        class AppComponent extends React.Component {


We still need this test because it tests both server and client.

hmm. Has been resolved, It can test passed both server and client. There is no warning.

sebmarkbage · 2017-07-21T02:41:15Z

Also, don't forget about iterables that are not arrays.

evilbs · 2017-07-21T03:09:28Z

@sebmarkbage , Thanks for reminding, "React.Children.toArray" might be a good method use to iterate that are not arrays.

gaearon · 2017-07-27T17:00:51Z

Looks like CI is failing, can you fix this?

evilbs · 2017-07-28T03:52:20Z

done, @gaearon , Ci is passed!

gaearon · 2017-07-28T15:29:20Z

src/renderers/shared/server/ReactPartialRenderer.js

@@ -487,6 +482,20 @@ class ReactDOMServerRenderer {
      if (child === null || child === false) {
        return '';
      } else {
+        var children = toArray(child);
+        if (children.length > 1) {


This breaks for arrays returning one element.

For example

function Foo() { return [<div />]; }

crashes.

gaearon · 2017-07-28T15:39:29Z

src/renderers/shared/server/ReactPartialRenderer.js

@@ -487,6 +482,20 @@ class ReactDOMServerRenderer {
      if (child === null || child === false) {
        return '';
      } else {
+        var children = toArray(child);


Let's not call toArray() unless we know this is not a single element. The case where single element is returned should be a fast one. For example return <Foo /> shouldn't wrap <Foo /> into array before traversing down.

gaearon

We need to fix the single array item case
We need to not call toArray() for single-child-without-array case

I think the right way to restructure the conditions is:

      } else if (isValidElement(child)) {
        return this.renderDOM(child, context);
      } else {
        var children = toArray(child);
        // ...

Please also add test cases for the single array element, and for nested arrays.

gaearon · 2017-07-28T15:47:56Z

We also need tests that verify that plain objects as children throw. Similar to this test. Maybe you can put it in the same place and add (ssr) to its name.

evilbs · 2017-07-29T04:05:44Z

👍 Thanks you for pointing, I understand what you mean, I will adjust these case as soon as.

evilbs · 2017-07-30T03:49:45Z

Changed done, Please review. @gaearon

gaearon · 2017-07-30T15:54:40Z

src/renderers/dom/ReactDOMNodeStreamRenderer.js

-  invariant(
-    React.isValidElement(element),
-    'renderToStream(): You must pass a valid ReactElement.',
-  );
  return new ReactMarkupReadableStream(element, false);


Seems like we also need to remove this check from renderToStaticStream as well?

gaearon · 2017-07-30T15:55:51Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

+    if (ReactDOMFeatureFlags.useFiber) {
+      itRenders('a array type children as a child', async render => {
+        let Header = props => {
+          return <p>header</p>;


Let's also make the component return array as well. This way we verify it's supported at multiple levels rather than just top level.

gaearon · 2017-07-30T15:56:54Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

+        expect(parent.childNodes[1].tagName).toBe('SPAN');
+      });
+
+      it('throws if a plain object is used as a child when using SSR', async () => {


Let's move this test to ReactComponent-test close to the non-SSR similar test.

gaearon · 2017-07-30T15:57:08Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

+        );
+      });
+
+      it('throws if a plain object even if it is in an owner when using SSR', async () => {


Same, let's move it close to identical non-SSR test.

gaearon

Minor changes.

evilbs · 2017-07-31T03:32:11Z

Changed done, Please review. @gaearon .

sorry, I forgot to set description when committing . 😸

gaearon · 2017-07-31T15:59:17Z

Thank you so much!

evilbs · 2017-08-01T02:45:48Z

😄 I am very happy that the pr merged, I am grateful for your help. I will continue to study and submit more pr.

fix(*): fix renderToString fails with array type children when react-…

790e65a

…dom/server render

facebook-github-bot added the CLA Signed label Jul 19, 2017

evilbs added 2 commits July 20, 2017 15:28

Merge remote-tracking branch 'upstream/master'

c2aeb06

Add an integration test that verify renderToString with array type ch…

6fd78e5

…ildren

guoyong added 2 commits July 21, 2017 00:36

Add integration test to renderToString with array type child

4b28bc1

Add integration test to renderToString with array type child

f652ed7

Merge remote-tracking branch 'origin/master'

322fa40

gaearon reviewed Jul 20, 2017

View reviewed changes

guoyong added 2 commits July 21, 2017 01:21

Update integration test to renderToString with array type child

deb11af

fix renderToString fails with array type children when react-dom/serv…

62c042e

…er render

gaearon requested changes Jul 20, 2017

View reviewed changes

Update integration test to renderToString with array type child

6b4eb0e

guoyong added 4 commits July 21, 2017 11:54

Add to iterate that are not arrays

e53e552

Add the validation react element

eafb4bd

Improve an integration test of server renderToString with array type

3df964d

Merge branch 'master' of https://github.com/facebook/react

6f7f142

guoyong added 3 commits July 28, 2017 10:48

Remove array validate

e0bd356

Merge branch 'master' of https://github.com/gzhappysky/react

1d45bae

prettier

5f8a2dd

gaearon reviewed Jul 28, 2017

View reviewed changes

gaearon requested changes Jul 28, 2017

View reviewed changes

guoyong added 3 commits July 29, 2017 18:58

Make SSR can handle a single array element and nested array

cf005dd

prettier

7f5c0a1

Change integeration test description

60e7e3e

gaearon reviewed Jul 30, 2017

View reviewed changes

gaearon requested changes Jul 30, 2017

View reviewed changes

modified: src/renderers/dom/ReactDOMNodeStreamRenderer.js

e4dd558

gaearon added 4 commits July 31, 2017 15:15

Make invariants consistent

59e0843

Gate outdated test itself by feature flag

5539f09

Change test to make sense both with old and new code

06d9971

Test more cases

b42dd08

gaearon approved these changes Jul 31, 2017

View reviewed changes

gaearon merged commit f732fc6 into facebook:master Jul 31, 2017

gaearon mentioned this pull request Aug 1, 2017

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

fix renderToString fails with array type children when react-dom/server render #10221

fix renderToString fails with array type children when react-dom/server render #10221

Conversation

evilbs commented Jul 19, 2017 • edited Loading

facebook-github-bot commented Jul 19, 2017

facebook-github-bot commented Jul 19, 2017

gaearon commented Jul 19, 2017 • edited Loading

evilbs commented Jul 19, 2017

evilbs commented Jul 20, 2017 • edited Loading

gaearon commented Jul 20, 2017 • edited Loading

evilbs commented Jul 20, 2017

gaearon commented Jul 20, 2017

gaearon commented Jul 20, 2017

gaearon commented Jul 20, 2017

evilbs commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilbs Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilbs Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilbs Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evilbs Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

sebmarkbage commented Jul 21, 2017

evilbs commented Jul 21, 2017 • edited Loading

gaearon commented Jul 27, 2017

evilbs commented Jul 28, 2017

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

Choose a reason for hiding this comment

gaearon commented Jul 28, 2017

evilbs commented Jul 29, 2017 • edited Loading

evilbs commented Jul 30, 2017

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

Choose a reason for hiding this comment

evilbs commented Jul 31, 2017

gaearon commented Jul 31, 2017

evilbs commented Aug 1, 2017 • edited Loading

evilbs commented Jul 19, 2017 •

edited

Loading

gaearon commented Jul 19, 2017 •

edited

Loading

evilbs commented Jul 20, 2017 •

edited

Loading

gaearon commented Jul 20, 2017 •

edited

Loading

evilbs Jul 20, 2017 •

edited

Loading

evilbs Jul 20, 2017 •

edited

Loading

evilbs Jul 21, 2017 •

edited

Loading

evilbs Jul 21, 2017 •

edited

Loading

evilbs commented Jul 21, 2017 •

edited

Loading

evilbs commented Jul 29, 2017 •

edited

Loading

evilbs commented Aug 1, 2017 •

edited

Loading