fix(ServerRendering): execution should be sync

The documentation states that React.renderComponentToString 'uses a callback API to keep the API async', but the implementation is actually synchronous. In order to maintain this contract the callback should always be called asynchronously or be change to a synchronous API. As per the discussion of pull request 982, the API should be changed to a synchronous one.
tommyinbi · Feb 3, 2014 · cd2aecc · cd2aecc
1 parent 526be15
commit cd2aecc
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 104 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -57,6 +57,7 @@ Facebook has a [bounty program](https://www.facebook.com/whitehat/) for the safe
 * `"use strict";`
 * 80 character line length
 * "Attractive"
+* Do not use the optional parameters of `setTimeout` and `setInterval`
 
 ## License
 

diff --git a/src/core/__tests__/ReactRenderDocument-test.js b/src/core/__tests__/ReactRenderDocument-test.js
@@ -64,14 +64,13 @@ describe('rendering React components at document', function() {
       }
     });
 
-    React.renderComponentToString(<Root />, function(markup) {
-      testDocument = getTestDocument(markup);
-      var component = React.renderComponent(<Root />, testDocument);
-      expect(testDocument.body.innerHTML).toBe('Hello world');
+    var markup = React.renderComponentToString(<Root />);
+    testDocument = getTestDocument(markup);
+    var component = React.renderComponent(<Root />, testDocument);
+    expect(testDocument.body.innerHTML).toBe('Hello world');
 
-      var componentID = ReactMount.getReactRootID(testDocument);
-      expect(componentID).toBe(component._rootNodeID);
-    });
+    var componentID = ReactMount.getReactRootID(testDocument);
+    expect(componentID).toBe(component._rootNodeID);
   });
 
   it('should not be able to unmount component from document node', function() {
@@ -92,17 +91,16 @@ describe('rendering React components at document', function() {
       }
     });
 
-    React.renderComponentToString(<Root />, function(markup) {
-      testDocument = getTestDocument(markup);
-      React.renderComponent(<Root />, testDocument);
-      expect(testDocument.body.innerHTML).toBe('Hello world');
+    var markup = React.renderComponentToString(<Root />);
+    testDocument = getTestDocument(markup);
+    React.renderComponent(<Root />, testDocument);
+    expect(testDocument.body.innerHTML).toBe('Hello world');
 
-      expect(function() {
-        React.unmountComponentAtNode(testDocument);
-      }).toThrow(UNMOUNT_INVARIANT_MESSAGE);
+    expect(function() {
+      React.unmountComponentAtNode(testDocument);
+    }).toThrow(UNMOUNT_INVARIANT_MESSAGE);
 
-      expect(testDocument.body.innerHTML).toBe('Hello world');
-    });
+    expect(testDocument.body.innerHTML).toBe('Hello world');
   });
 
   it('should not be able to switch root constructors', function() {
@@ -138,20 +136,19 @@ describe('rendering React components at document', function() {
       }
     });
 
-    React.renderComponentToString(<Component />, function(markup) {
-      testDocument = getTestDocument(markup);
+    var markup = React.renderComponentToString(<Component />);
+    testDocument = getTestDocument(markup);
 
-      React.renderComponent(<Component />, testDocument);
+    React.renderComponent(<Component />, testDocument);
 
-      expect(testDocument.body.innerHTML).toBe('Hello world');
+    expect(testDocument.body.innerHTML).toBe('Hello world');
 
-      // Reactive update
-      expect(function() {
-        React.renderComponent(<Component2 />, testDocument);
-      }).toThrow(UNMOUNT_INVARIANT_MESSAGE);
+    // Reactive update
+    expect(function() {
+      React.renderComponent(<Component2 />, testDocument);
+    }).toThrow(UNMOUNT_INVARIANT_MESSAGE);
 
-      expect(testDocument.body.innerHTML).toBe('Hello world');
-    });
+    expect(testDocument.body.innerHTML).toBe('Hello world');
   });
 
   it('should be able to mount into document', function() {
@@ -172,16 +169,14 @@ describe('rendering React components at document', function() {
       }
     });
 
-    React.renderComponentToString(
-      <Component text="Hello world" />,
-      function(markup) {
-        testDocument = getTestDocument(markup);
+    var markup = React.renderComponentToString(
+      <Component text="Hello world" />
+    );
+    testDocument = getTestDocument(markup);
 
-        React.renderComponent(<Component text="Hello world" />, testDocument);
+    React.renderComponent(<Component text="Hello world" />, testDocument);
 
-        expect(testDocument.body.innerHTML).toBe('Hello world');
-      }
-    );
+    expect(testDocument.body.innerHTML).toBe('Hello world');
   });
 
   it('should give helpful errors on state desync', function() {
@@ -202,26 +197,24 @@ describe('rendering React components at document', function() {
       }
     });
 
-    React.renderComponentToString(
-      <Component text="Goodbye world" />,
-      function(markup) {
-        testDocument = getTestDocument(markup);
-
-        expect(function() {
-          // Notice the text is different!
-          React.renderComponent(<Component text="Hello world" />, testDocument);
-        }).toThrow(
-          'Invariant Violation: ' +
-          'You\'re trying to render a component to the document using ' +
-          'server rendering but the checksum was invalid. This usually ' +
-          'means you rendered a different component type or props on ' +
-          'the client from the one on the server, or your render() methods ' +
-          'are impure. React cannot handle this case due to cross-browser ' +
-          'quirks by rendering at the document root. You should look for ' +
-          'environment dependent code in your components and ensure ' +
-          'the props are the same client and server side.'
-        );
-      }
+    var markup = React.renderComponentToString(
+      <Component text="Goodbye world" />
+    );
+    testDocument = getTestDocument(markup);
+
+    expect(function() {
+      // Notice the text is different!
+      React.renderComponent(<Component text="Hello world" />, testDocument);
+    }).toThrow(
+      'Invariant Violation: ' +
+      'You\'re trying to render a component to the document using ' +
+      'server rendering but the checksum was invalid. This usually ' +
+      'means you rendered a different component type or props on ' +
+      'the client from the one on the server, or your render() methods ' +
+      'are impure. React cannot handle this case due to cross-browser ' +
+      'quirks by rendering at the document root. You should look for ' +
+      'environment dependent code in your components and ensure ' +
+      'the props are the same client and server side.'
     );
   });
 

diff --git a/src/core/__tests__/ReactTextComponent-test.js b/src/core/__tests__/ReactTextComponent-test.js
@@ -30,9 +30,8 @@ describe('ReactTextComponent', function() {
     var ThisThingShouldBeEscaped = '">>> LULZ <<<"';
     var ThisThingWasBeEscaped = '&quot;&gt;&gt;&gt; LULZ &lt;&lt;&lt;&quot;';
     var thing = React.DOM.div(null, React.DOM.span({key:ThisThingShouldBeEscaped}, ["LULZ"]));
-    React.renderComponentToString(thing, function(html){
-      expect(html).not.toContain(ThisThingShouldBeEscaped);
-      expect(html).toContain(ThisThingWasBeEscaped);
-    });
+    var html = React.renderComponentToString(thing);
+    expect(html).not.toContain(ThisThingShouldBeEscaped);
+    expect(html).toContain(ThisThingWasBeEscaped);
   })
 });
diff --git a/src/environment/ReactServerRendering.js b/src/environment/ReactServerRendering.js
@@ -27,30 +27,27 @@ var invariant = require('invariant');
 
 /**
  * @param {ReactComponent} component
- * @param {function} callback
+ * @return {String} the markup
  */
-function renderComponentToString(component, callback) {
-  // We use a callback API to keep the API async in case in the future we ever
-  // need it, but in reality this is a synchronous operation.
-
+function renderComponentToString(component) {
   invariant(
     ReactComponent.isValidComponent(component),
     'renderComponentToString(): You must pass a valid ReactComponent.'
   );
 
   invariant(
-    typeof callback === 'function',
-    'renderComponentToString(): You must pass a function as a callback.'
+    !(arguments.length === 2 && typeof arguments[1] === 'function'),
+    'renderComponentToString(): This function became synchronous and now ' +
+    'returns the generated markup. Please remove the second parameter.'
   );
 
   var id = ReactInstanceHandles.createReactRootID();
   var transaction = ReactReconcileTransaction.getPooled();
   transaction.reinitializeTransaction();
   try {
-    transaction.perform(function() {
+    return transaction.perform(function() {
       var markup = component.mountComponent(id, transaction, 0);
-      markup = ReactMarkupChecksum.addChecksumToMarkup(markup);
-      callback(markup);
+      return ReactMarkupChecksum.addChecksumToMarkup(markup);
     }, null);
   } finally {
     ReactReconcileTransaction.release(transaction);

diff --git a/src/environment/__tests__/ReactServerRendering-test.js b/src/environment/__tests__/ReactServerRendering-test.js
@@ -56,12 +56,8 @@ describe('ReactServerRendering', function() {
   });
 
   it('should generate simple markup', function() {
-    var response;
-    ReactServerRendering.renderComponentToString(
-      <span>hello world</span>,
-      function(response_) {
-        response = response_;
-      }
+    var response = ReactServerRendering.renderComponentToString(
+      <span>hello world</span>
     );
     expect(response).toMatch(
       '<span ' + ID_ATTRIBUTE_NAME + '="[^"]+" ' +
@@ -73,12 +69,10 @@ describe('ReactServerRendering', function() {
     var EventPluginHub = require('EventPluginHub');
     var cb = mocks.getMockFunction();
 
-    ReactServerRendering.renderComponentToString(
-      <span onClick={cb}>hello world</span>,
-      function() {
-        expect(EventPluginHub.__getListenerBank()).toEqual({});
-      }
+    var response = ReactServerRendering.renderComponentToString(
+      <span onClick={cb}>hello world</span>
     );
+    expect(EventPluginHub.__getListenerBank()).toEqual({});
   });
 
   it('should render composite components', function() {
@@ -92,12 +86,8 @@ describe('ReactServerRendering', function() {
         return <span>My name is {this.props.name}</span>;
       }
     });
-    var response;
-    ReactServerRendering.renderComponentToString(
-      <Parent />,
-      function(response_) {
-        response = response_;
-      }
+    var response = ReactServerRendering.renderComponentToString(
+      <Parent />
     );
     expect(response).toMatch(
       '<div ' + ID_ATTRIBUTE_NAME + '="[^"]+" ' +
@@ -144,13 +134,8 @@ describe('ReactServerRendering', function() {
       }
     });
 
-    var response;
-
-    ReactServerRendering.renderComponentToString(
-      <TestComponent />,
-      function (_response) {
-        response = _response;
-      }
+    var response = ReactServerRendering.renderComponentToString(
+      <TestComponent />
     );
 
     expect(response).toMatch(
@@ -212,14 +197,10 @@ describe('ReactServerRendering', function() {
     expect(element.innerHTML).toEqual('');
 
     ExecutionEnvironment.canUseDOM = false;
-    ReactServerRendering.renderComponentToString(
-      <TestComponent name="x" />,
-      function(markup) {
-        lastMarkup = markup;
-      }
+    lastMarkup = ReactServerRendering.renderComponentToString(
+      <TestComponent name="x" />
     );
     ExecutionEnvironment.canUseDOM = true;
-
     element.innerHTML = lastMarkup + ' __sentinel__';
 
     React.renderComponent(<TestComponent name="x" />, element);
@@ -250,23 +231,25 @@ describe('ReactServerRendering', function() {
     expect(
       ReactServerRendering.renderComponentToString.bind(
         ReactServerRendering,
-        'not a component',
-        function() {}
+        'not a component'
       )
     ).toThrow(
       'Invariant Violation: renderComponentToString(): You must pass ' +
       'a valid ReactComponent.'
     );
+  });
 
+  it('should provide guidance for breaking API changes', function() {
     expect(
       ReactServerRendering.renderComponentToString.bind(
         ReactServerRendering,
-        React.DOM.div(),
-        'not a function'
+        <div />,
+        function(){}
       )
     ).toThrow(
-      'Invariant Violation: renderComponentToString(): You must pass ' +
-      'a function as a callback.'
+      'Invariant Violation: renderComponentToString(): This function became ' +
+      'synchronous and now returns the generated markup. Please remove the ' +
+      'second parameter.'
     );
   });
 });