fix(ServerRendering): execution should be async #982

bripkens · 2014-01-28T19:01:33Z

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.

zpao · 2014-01-28T19:18:08Z

src/environment/ReactServerRendering.js

@@ -50,7 +50,7 @@ function renderComponentToString(component, callback) {
    transaction.perform(function() {
      var markup = component.mountComponent(id, transaction, 0);
      markup = ReactMarkupChecksum.addChecksumToMarkup(markup);
-      callback(markup);
+      window.setTimeout(callback, 0, markup);


window doesn't necessarily exist where you might call this.

bripkens · 2014-01-28T19:25:26Z

Very true, sorry for that. Updated the PR accordingly.

syranide · 2014-01-28T22:09:43Z

@bripkens FYI, I think you forgot to push it.

cpojer · 2014-01-28T23:02:04Z

src/environment/ReactServerRendering.js

@@ -50,7 +50,7 @@ function renderComponentToString(component, callback) {
    transaction.perform(function() {
      var markup = component.mountComponent(id, transaction, 0);
      markup = ReactMarkupChecksum.addChecksumToMarkup(markup);
-      callback(markup);
+      setTimeout(callback, 0, markup);


Can you change this to setTimeout(() => callback(markup), 0) ? Add FB we disallow to use the third argument for setTimeout like this.

bripkens · 2014-01-29T04:24:03Z

Done. CLA is signed as well.

johnthethird · 2014-01-29T22:40:56Z

I recently implemented server-side rendering in Rails here:

https://github.com/johnthethird/react-rails/blob/server-rendering/lib/react/rails/view_helper.rb#L71

It is a lot easier if renderComponentToString is fake-async like it used to be. Getting data out of a JS VM like V8 or Rhino (Java) is difficult when it comes to async APIs. For example ExecJS which Rails/Sprockets uses to wrap different JS VMs does not support it at all.

What if renderComponentToString, in addition to the truly async callback, also just plain returned the HTML?

petehunt · 2014-01-29T22:42:35Z

What if we just committed to a sync API? Do we think this is likely to realistically change in the near term @sebmarkbage @jordwalke ? I know we've talked about async rendering but it seems pretty far off.

cpojer · 2014-01-29T22:50:15Z

woops, sorry, don't ask me what happened there.

sebmarkbage · 2014-01-29T23:04:44Z

@petehunt I'd be fine with that. A short term async rendering would likely be of the continuation style or allow certain components to render a partial result and then update themselves. Longterm, however, we could do parallelized rendering with PJS. All could be opt-in.

dazld · 2014-01-30T00:25:36Z

src/environment/ReactServerRendering.js

@@ -50,7 +50,7 @@ function renderComponentToString(component, callback) {
    transaction.perform(function() {
      var markup = component.mountComponent(id, transaction, 0);
      markup = ReactMarkupChecksum.addChecksumToMarkup(markup);
-      callback(markup);
+      setTimeout(() => callback(markup), 0);


process.nextTick would be more useful on the server...? why zero timeout?

I didn't mean to restrict the server side rendering to node.js. For instance, I am currently trying to make this work on the JVM where process.nextTick is not available.

I think a lot more people use node than other server side js environments, and even then, could shim this to use node conventions as default, falling back to setTimeout for other platforms. (very personal bias though!)

The semantic difference between process.nextTick and setTimeout seems negligible in this case (while it might not be for edge cases). I would therefore argue that compatibility is more important than Node conventions (especially considering that renderComponentToString is part of React's default distribution and not part of some server side add-on).

Anyhow, let's wait until the core team has made up their mind w.r.t. asynchronous rendering in general :-).

true that, anything that runs on client would need to avoid nodeisms. I think I saw somewhere a comment about using setImmediate..? there's a nice discussion of the problem with setTimeout's clamped minimum values vs real nextTick deferral here: http://jphpsf.github.io/setImmediate-shim-demo/

bripkens · 2014-01-30T06:24:02Z

Maybe I should provide some context and why I think async rendering is interesting.

Over the last two weeks I have been playing with the idea of server side rendering. Rendering simple component trees is easy with the current version of renderComponentToString, but many applications will probably need to retrieve some data in order to display it. As of now, the implementation of renderComponentToString is not capable of handling such cases and may therefore be synchronous (as proposed by @petehunt).

I stumbled over this issue while making facebookarchive/react-page#47 work on the JVM. On the JVM I have complete control over the event loop and can identify the moment when all data has been loaded. I am hoping to make server side rendering work without any changes to an existing React application. That being said, more React changes will be necessary in order to support this "advanced" server side rendering (and then the question remains whether these changes are portable across JS environments).

bripkens · 2014-01-30T06:28:54Z

Just noticed #985.

Supporting server side rendering cases with async data retrieval is surely going to require a fair amount of work. If you want to make the API synchronous and thus not focus on this issue now, I have no problem with that.

andreypopp · 2014-01-30T11:31:56Z

On a related note, what about changing the callback signature to callback(err, markup) like in Node.js and propagate all errors through the callback? It's a little bit inconsistent, I think, to expect a result in a callback and an error via try ... catch ... statement. What do you think?

dazld · 2014-01-30T11:40:42Z

following node conventions seems like a good idea.

petehunt · 2014-01-30T18:43:09Z

I think that data fetching is workable even in a sync world. You can just move the asynchronicity outside of React. You can even put the code for data fetching alongside your React component, just don't call render() until all the data is fetched and pass the data in as a top-level prop.

I think making this method synchronous would be better.

bripkens · 2014-02-02T09:02:47Z

Would you like me to update my PR accordingly, i.e. change the API to synchronous, or do you wish to wait with this (breaking) change? Would love to help you out here.

petehunt · 2014-02-02T22:02:16Z

@bripkens if you want to make the API synchronous that would be awesome!

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.

bripkens · 2014-02-03T10:43:11Z

@petehunt PR updated. API is no synchronous and provides some guidance for migrations.

zpao · 2014-02-03T18:35:34Z

src/environment/ReactServerRendering.js

-  // 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(


I'm not sure that this should be an invariant... do we want to stop execution or should we let it keep running and just warn? @petehunt?

I am cool with this

…eckers

bripkens · 2014-02-04T08:08:23Z

Sure thing, here you go. The word "internal" might imply that it is not, but is this typechecker available somewhere? You made me curious ;-).

petehunt · 2014-02-04T18:24:20Z

It's not available at this point, sorry :/

fix(ServerRendering): execution should be async

plievone · 2014-02-05T21:16:19Z

@bripkens Needs docs changes here? https://github.com/facebook/react/blob/master/docs/docs/ref-01-top-level-api.md

bripkens · 2014-02-06T06:58:19Z

It does, yes. I opened PR #1033 with the necessary doc changes.

See the background in #982. This PR reimplements part of InferReactiveScopes, aligning reactive scopes to block boundaries, but against ReactiveFunction instead of the HIR.

See the background in #982. This PR reimplements part of InferReactiveScopes, merging overlapping reactive scopes, but against ReactiveFunction instead of the HIR.

zpao reviewed Jan 28, 2014
View reviewed changes

cpojer reviewed Jan 28, 2014
View reviewed changes

cpojer closed this Jan 29, 2014

cpojer reopened this Jan 29, 2014

dazld reviewed Jan 30, 2014
View reviewed changes

zpao reviewed Feb 3, 2014
View reviewed changes

Use lowercase "string" so that the documentation complies with typech…

e14555c

…eckers

petehunt added a commit that referenced this pull request Feb 4, 2014

Merge pull request #982 from bripkens/async-tostring

3642b6e

fix(ServerRendering): execution should be async

petehunt merged commit 3642b6e into facebook:master Feb 4, 2014

bripkens mentioned this pull request Feb 6, 2014

docs(ServerRendering): Reflect renderComponentToString changes #1033

Merged

wwwsevolod mentioned this pull request May 30, 2014

Node Fibers andreypopp/react-async#7

Closed

burabure mentioned this pull request Apr 30, 2015

Issue with server rendering and HoC acdlite/flummox#170

Closed

ryan-ally mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 ryan-ally/react-1#238

Open

jackromo888 mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 jackromo888/react#245

Open

AlexanderGrovnes mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 AlexanderGrovnes/react#40

Open

ryan-ally mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.3.0 to 9.1.3 ryan-ally/react-1#239

Open

DevProp mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade babel-loader from 8.1.0 to 9.1.3 DevProp/react#40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ServerRendering): execution should be async #982

fix(ServerRendering): execution should be async #982

bripkens commented Jan 28, 2014

zpao Jan 28, 2014

bripkens commented Jan 28, 2014

syranide commented Jan 28, 2014

cpojer Jan 28, 2014

bripkens commented Jan 29, 2014

johnthethird commented Jan 29, 2014

petehunt commented Jan 29, 2014

cpojer commented Jan 29, 2014

sebmarkbage commented Jan 29, 2014

dazld Jan 30, 2014

bripkens Jan 30, 2014

dazld Jan 30, 2014

bripkens Jan 30, 2014

dazld Jan 30, 2014

bripkens commented Jan 30, 2014

bripkens commented Jan 30, 2014

andreypopp commented Jan 30, 2014

dazld commented Jan 30, 2014

petehunt commented Jan 30, 2014

bripkens commented Feb 2, 2014

petehunt commented Feb 2, 2014

bripkens commented Feb 3, 2014

zpao Feb 3, 2014

petehunt Feb 3, 2014

bripkens commented Feb 4, 2014

petehunt commented Feb 4, 2014

plievone commented Feb 5, 2014

bripkens commented Feb 6, 2014

fix(ServerRendering): execution should be async #982

fix(ServerRendering): execution should be async #982

Conversation

bripkens commented Jan 28, 2014

Choose a reason for hiding this comment

bripkens commented Jan 28, 2014

syranide commented Jan 28, 2014

Choose a reason for hiding this comment

bripkens commented Jan 29, 2014

johnthethird commented Jan 29, 2014

petehunt commented Jan 29, 2014

cpojer commented Jan 29, 2014

sebmarkbage commented Jan 29, 2014

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

bripkens commented Jan 30, 2014

bripkens commented Jan 30, 2014

andreypopp commented Jan 30, 2014

dazld commented Jan 30, 2014

petehunt commented Jan 30, 2014

bripkens commented Feb 2, 2014

petehunt commented Feb 2, 2014

bripkens commented Feb 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bripkens commented Feb 4, 2014

petehunt commented Feb 4, 2014

plievone commented Feb 5, 2014

bripkens commented Feb 6, 2014