Skip to content
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

Feature : can render as stream #303

Closed
wants to merge 1 commit into from
Closed

Conversation

formula1
Copy link
Collaborator

@formula1 formula1 commented Aug 8, 2016

This is something that has been talked about many times on the react github (facebook/react#3009, https://github.com/facebook/react/search?q=stream+server&type=Issues&utf8=%E2%9C%93). I may make a pull request there as well but their codebase is rather insane. Your existing renderer is pretty solid and simple so I figured it wouldn't be too difficult re-implementing it as a stream.

Some important notes

  • I'm running componentWillMount as if it will return a promise - This is an entirely seperate issue that I would like to bring up regarding isomorphic behaviour and updates.
  • I am not rendering child components in parrallel - The reason for this isn't quite so simple. Effectively, we don't know how long each child will take to render. So we would need to give a stream to each child and pipe them to the parents stream as the previous finishes. If a later sibling finishes before a previous that is none of our concern since order is more important than the time it takes for a sibling to finish. Below is some psuedo code regarding this concept
  • I basically copied the renderToString tests - I wasn't particularly interested in recreating the wheel or figuring out additional. Basically just wanted to spend 2 to 3 hours on a neat project
var parentStream = this;
var taken = false;
var lastChild = -1;
var waitingList = {};

Promise.all(children.map((child, i) =>{
  // we don't know how long a child will take to render
  // we expect them to be done in proper order
  return prepareChildStream(child).then((stream) =>{
    if(taken === false){
      if(lastChild === i - 1) consumeStream(parentStream, stream, i, waitingList);
    }
    waitingList[ i ] = stream;
  });
})

function consumeStream(parentStream, childStream, childIndex, waitingList){
  taken = true;
  childStream.on('end', () =>{
    // we don't want to have a childStream to end the mainStream
    childStream.unpipe(parentStream);
    lastChild = childIndex;
    // if the next one is available, run it
    if(waitingList[ childIndex + 1 ]) consumeStream(parentStream, waitingList[ childIndex + 1], i);
    // otherwise allow the next one to run once they are ready
    else taken = false;
  }).pipe(parentStream)
}

@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.06%) to 95.866% when pulling 2091a94 on formula1:master into 2787e8d on trueadm:master.

@trueadm
Copy link
Member

trueadm commented Aug 8, 2016

@formula1 This is awesome stuff! I'll review in detail later – but could you possibly re-create the PR against the dev branch rather than master? Also, as you may be aware, going forward 0.8 will be a big revamp in terms of tidying up the code and fixing many issues – it would be interesting to to see how this PR can easily be merged into 0.8 too. Also – are you on the Inferno Slack? If not, please can you email me your email address so I could add you (email me at dg @ domgan.com).

Thanks again!

@formula1
Copy link
Collaborator Author

formula1 commented Aug 8, 2016

Absolutely, I'll look more into it tonight. Already I can tell that the basic code is nearly identical so it should be no issue. A few things I'd like to do though is share as much between my code and what already exists so that fixes are applied to both places. Again, I'll look more into this tonight. I find this project particularly exciting

@formula1 formula1 closed this Aug 8, 2016
This was referenced Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants