New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement data-fetching for server rendering #51

Open
mhaagens opened this Issue Mar 2, 2017 · 19 comments

Comments

Projects
None yet
8 participants
@mhaagens

mhaagens commented Mar 2, 2017

This is more of a question, but would it be possible to implement something like this into Rapscallion? https://github.com/rexhome7326/sync-will-mount

Tested it and it works, lets you await a promise in ComponentWillMount() before continuing render.
With React Router 4's new API data fetching is a little more cumbersome, so a lot of people are looking for simpler solutions to server side data fetching.

@mhaagens

This comment has been minimized.

Show comment
Hide comment
@mhaagens

mhaagens Mar 2, 2017

Something like this (in traverse.js)? Just tried it out and it seems to work.
Lifted the code from sync-will-mount, can probably change it to use async/await etc...

function evalComponent (seq, node, context) {
  const componentContext = getContext(node.type, context);

  if (!(node.type.prototype && node.type.prototype.isReactComponent)) {
    const instance = node.type(node.props, componentContext);
    const childContext = getChildContext(node.type, instance, context);
    traverse(seq, instance, childContext);
    return;
  }

  // eslint-disable-next-line new-cap
  const instance = new node.type(node.props, componentContext);

  let res = null;
  let promise = null;

  if (typeof instance.componentWillMount === "function") {
    instance.setState = syncSetState;
    res = instance.componentWillMount();
  }

  if (res && res.then) {
    promise = res
  }

  let done = false;

  if(promise){
    promise.then(() => {done = true;});
  } else {
    done = true;
  }
  
  require('deasync').loopWhile(function(){return !done;});

  if (done) {
    const childContext = getChildContext(node.type, instance, context);
    traverse(seq, instance.render(), childContext);
  }

}

mhaagens commented Mar 2, 2017

Something like this (in traverse.js)? Just tried it out and it seems to work.
Lifted the code from sync-will-mount, can probably change it to use async/await etc...

function evalComponent (seq, node, context) {
  const componentContext = getContext(node.type, context);

  if (!(node.type.prototype && node.type.prototype.isReactComponent)) {
    const instance = node.type(node.props, componentContext);
    const childContext = getChildContext(node.type, instance, context);
    traverse(seq, instance, childContext);
    return;
  }

  // eslint-disable-next-line new-cap
  const instance = new node.type(node.props, componentContext);

  let res = null;
  let promise = null;

  if (typeof instance.componentWillMount === "function") {
    instance.setState = syncSetState;
    res = instance.componentWillMount();
  }

  if (res && res.then) {
    promise = res
  }

  let done = false;

  if(promise){
    promise.then(() => {done = true;});
  } else {
    done = true;
  }
  
  require('deasync').loopWhile(function(){return !done;});

  if (done) {
    const childContext = getChildContext(node.type, instance, context);
    traverse(seq, instance.render(), childContext);
  }

}
@abritinthebay

This comment has been minimized.

Show comment
Hide comment
@abritinthebay

abritinthebay Mar 15, 2017

Feels like this would be a good candidate to explore optional Rapscallion plugins/middleware. It's a very workflow specific thing to have to have in the main library.

abritinthebay commented Mar 15, 2017

Feels like this would be a good candidate to explore optional Rapscallion plugins/middleware. It's a very workflow specific thing to have to have in the main library.

@djgrant

This comment has been minimized.

Show comment
Hide comment
@djgrant

djgrant Mar 15, 2017

It is non-standard but there is definitely general demand for a feature like this. For example, the apollo client currently traverses a react tree before every render to resolve data dependencies (if that could be done during SSR it would introduce a great performance boost to many graphql apps). Other libraries have introduced functionality to solve the same issue.

I think the approach that is really emerging as a convention within the community is to extend the component contract with getInitialProps. Both next.js (https://github.com/zeit/next.js/#fetching-data-and-component-lifecycle) and inferno (https://infernojs.org/docs/api/inferno-server) are doing this.

@stubailo @jbaxleyiii might be able to offer some thoughts on this from the perspective of a library author.

djgrant commented Mar 15, 2017

It is non-standard but there is definitely general demand for a feature like this. For example, the apollo client currently traverses a react tree before every render to resolve data dependencies (if that could be done during SSR it would introduce a great performance boost to many graphql apps). Other libraries have introduced functionality to solve the same issue.

I think the approach that is really emerging as a convention within the community is to extend the component contract with getInitialProps. Both next.js (https://github.com/zeit/next.js/#fetching-data-and-component-lifecycle) and inferno (https://infernojs.org/docs/api/inferno-server) are doing this.

@stubailo @jbaxleyiii might be able to offer some thoughts on this from the perspective of a library author.

@divmain

This comment has been minimized.

Show comment
Hide comment
@divmain

divmain Mar 15, 2017

Collaborator

Thanks for pointing me to getInitialProps @djgrant! That's exactly what I was looking for. I'm adding this feature request into the queue.

Collaborator

divmain commented Mar 15, 2017

Thanks for pointing me to getInitialProps @djgrant! That's exactly what I was looking for. I'm adding this feature request into the queue.

@divmain divmain self-assigned this Mar 15, 2017

@divmain divmain added the enhancement label Mar 15, 2017

@abritinthebay

This comment has been minimized.

Show comment
Hide comment
@abritinthebay

abritinthebay Mar 15, 2017

Oh don't get me wrong - it's a cool feature. Was just questioning if it should be a core feature. Feels very... extend-y. That's usually the domain of plugins, especially when it's not core to the features of a project.

Of course that depends on project goals/architecture choices.

abritinthebay commented Mar 15, 2017

Oh don't get me wrong - it's a cool feature. Was just questioning if it should be a core feature. Feels very... extend-y. That's usually the domain of plugins, especially when it's not core to the features of a project.

Of course that depends on project goals/architecture choices.

@divmain

This comment has been minimized.

Show comment
Hide comment
@divmain

divmain Mar 15, 2017

Collaborator

@abritinthebay I'm open to exposing a generic plugin interface. My concern with that, however, is that it will be difficult to implement in a way that does not hurt performance. If you have thoughts on how to accomplish that, I'd be eager to hear them. In any event, the implementation will be as non-specific as we can manage without hurting perf.

Collaborator

divmain commented Mar 15, 2017

@abritinthebay I'm open to exposing a generic plugin interface. My concern with that, however, is that it will be difficult to implement in a way that does not hurt performance. If you have thoughts on how to accomplish that, I'd be eager to hear them. In any event, the implementation will be as non-specific as we can manage without hurting perf.

@abritinthebay

This comment has been minimized.

Show comment
Hide comment
@abritinthebay

abritinthebay Mar 15, 2017

True, though I'd argue the same is probably in this case - it's all going to have hot-path impact no matter what.

abritinthebay commented Mar 15, 2017

True, though I'd argue the same is probably in this case - it's all going to have hot-path impact no matter what.

@stubailo

This comment has been minimized.

Show comment
Hide comment
@stubailo

stubailo Mar 16, 2017

Yeah I think our approach is something similar to getInitialProps, but you can put it anywhere in the tree. I think it's definitely desirable that SSR is possible without requiring a static mapping of URL -> data fetch.

stubailo commented Mar 16, 2017

Yeah I think our approach is something similar to getInitialProps, but you can put it anywhere in the tree. I think it's definitely desirable that SSR is possible without requiring a static mapping of URL -> data fetch.

@djgrant

This comment has been minimized.

Show comment
Hide comment
@djgrant

djgrant Mar 16, 2017

@divmain @abritinthebay How about allowing a visitor function (something similar to https://github.com/ctrlplusb/react-tree-walker) to be attached to the renderer?

render(<App />)
  .attachVisitor(function visitor(element, instance, context) {
    if (instance.getInitialProps) {
      // returning a promise pauses traversal until promise resolves
      return instance.getInitialProps().then(initialProps => {
         instance.props = initialProps
      })
    }
  })
  .toPromise()

That would provide a flexible generic interface, and common visitors such as getInitialPropsVisitor could be packaged for developer convenience.

cc. @ctrlplusb

djgrant commented Mar 16, 2017

@divmain @abritinthebay How about allowing a visitor function (something similar to https://github.com/ctrlplusb/react-tree-walker) to be attached to the renderer?

render(<App />)
  .attachVisitor(function visitor(element, instance, context) {
    if (instance.getInitialProps) {
      // returning a promise pauses traversal until promise resolves
      return instance.getInitialProps().then(initialProps => {
         instance.props = initialProps
      })
    }
  })
  .toPromise()

That would provide a flexible generic interface, and common visitors such as getInitialPropsVisitor could be packaged for developer convenience.

cc. @ctrlplusb

@divmain

This comment has been minimized.

Show comment
Hide comment
@divmain

divmain Mar 17, 2017

Collaborator

That seems a reasonable approach. I'll look into whether this approach adds any performance overhead. It should be possible to add this without impacting perf for those not using the feature...

Collaborator

divmain commented Mar 17, 2017

That seems a reasonable approach. I'll look into whether this approach adds any performance overhead. It should be possible to add this without impacting perf for those not using the feature...

@divmain divmain changed the title from Question: Implement data-fetching for server rendering to Implement data-fetching for server rendering Mar 24, 2017

@ctrlplusb

This comment has been minimized.

Show comment
Hide comment
@ctrlplusb

ctrlplusb Apr 4, 2017

Hey all!

My promise-based API is now officially released for react-tree-walker. Seems to work well, but like @divmain states, it would be interesting to do some performance overhead testing on this.

I also released react-async-bootstrapper which is an abstraction on top of react-tree-walker and essentially provides what you guys are after, except it looks for a member called asyncBootstrap rather than getInitialProps.

I am using this with great success within react-async-component, which has been applied to the next branch of my server side rendering starter kit, react-universally.

Sorry, that is a mess of links. 😀

ctrlplusb commented Apr 4, 2017

Hey all!

My promise-based API is now officially released for react-tree-walker. Seems to work well, but like @divmain states, it would be interesting to do some performance overhead testing on this.

I also released react-async-bootstrapper which is an abstraction on top of react-tree-walker and essentially provides what you guys are after, except it looks for a member called asyncBootstrap rather than getInitialProps.

I am using this with great success within react-async-component, which has been applied to the next branch of my server side rendering starter kit, react-universally.

Sorry, that is a mess of links. 😀

@elierotenberg

This comment has been minimized.

Show comment
Hide comment
@elierotenberg

elierotenberg Jul 26, 2017

Maybe my own attempt at solving this issue can be helpful here:
https://github.com/elierotenberg/react-prepare

Instead of hacking the React lifecycle functions, I use a specially marked higher-order component, which is detected during the recursive traversal of the tree. I think separating the preparation step from the rendering step is usually a good idea, and I could implement a stream-friendly API so that it works nice with raspscallion.

elierotenberg commented Jul 26, 2017

Maybe my own attempt at solving this issue can be helpful here:
https://github.com/elierotenberg/react-prepare

Instead of hacking the React lifecycle functions, I use a specially marked higher-order component, which is detected during the recursive traversal of the tree. I think separating the preparation step from the rendering step is usually a good idea, and I could implement a stream-friendly API so that it works nice with raspscallion.

@elierotenberg

This comment has been minimized.

Show comment
Hide comment
@elierotenberg

elierotenberg Jul 26, 2017

Also, https://github.com/elierotenberg/react-traverse performs deep components output rewriting, using higher order magic.

Sorry for the plugs, but I think this can shed a light on various approaches to tackle this problem.

elierotenberg commented Jul 26, 2017

Also, https://github.com/elierotenberg/react-traverse performs deep components output rewriting, using higher order magic.

Sorry for the plugs, but I think this can shed a light on various approaches to tackle this problem.

@djgrant

This comment has been minimized.

Show comment
Hide comment
@djgrant

djgrant Jul 26, 2017

Nice! Thanks for sharing @elierotenberg.

I think separating the preparation step from the rendering step is usually a good idea

This means traversing the React tree twice. In my experiments that was always slower than resolving async work while rendering.

If we go ahead with a plugin approach (e.g. #51 (comment)) then you could write a plugin that conforms to your projects' APIs.

djgrant commented Jul 26, 2017

Nice! Thanks for sharing @elierotenberg.

I think separating the preparation step from the rendering step is usually a good idea

This means traversing the React tree twice. In my experiments that was always slower than resolving async work while rendering.

If we go ahead with a plugin approach (e.g. #51 (comment)) then you could write a plugin that conforms to your projects' APIs.

@ctrlplusb

This comment has been minimized.

Show comment
Hide comment
@ctrlplusb

ctrlplusb Jul 26, 2017

Will fiber land and change everything?

ctrlplusb commented Jul 26, 2017

Will fiber land and change everything?

@ctrlplusb

This comment has been minimized.

Show comment
Hide comment

ctrlplusb commented Jul 26, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 26, 2017

To be clear “asynchronous” Fiber features are not related to server side rendering. They’re related to client-side updates. The new server side renderer is not even using Fiber, and is written as standalone renderer with a simple while loop.

gaearon commented Jul 26, 2017

To be clear “asynchronous” Fiber features are not related to server side rendering. They’re related to client-side updates. The new server side renderer is not even using Fiber, and is written as standalone renderer with a simple while loop.

@djgrant

This comment has been minimized.

Show comment
Hide comment
@djgrant

djgrant Jul 26, 2017

@gaearon Thanks for the clarification! Is a server side renderer that is capable of doing async work during rendering likely to fall on the React roadmap in the future?

djgrant commented Jul 26, 2017

@gaearon Thanks for the clarification! Is a server side renderer that is capable of doing async work during rendering likely to fall on the React roadmap in the future?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 26, 2017

It might. @sebmarkbage was looking into this.

gaearon commented Jul 26, 2017

It might. @sebmarkbage was looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment