renderToNodeStream the full app without renderToString

[unleashit]

Currently, renderToString is being used to generate the app before passing it to renderToNodeStream. This instead passes the App down the tree as a react component so (theoretically) it can be streamed to the browser as it becomes available.

I've also fixed a small bug in /internal/development/index.js (config import is wrong).

[diondirza]

This is really great to be implemented, yet it requires some changes to make it fully work

[unleashit]

Thanks for looking at this. You're right about the prop-types but I'm not sure why you think it doesn't work. I tested in both dev and production, javascript on and off (to compare client/server markup) and made sure code splitting was working. Everything was fine on my end. But I made all the changes you asked for and it works that way too 👍

I think you're right that its aesthetically better to keep the wrapper div (with id="app") in /client/index.js. But not sure why moving the App call into the .then() of asyncbootstrapper would make a difference? Does AB mutate its input (app is different post bootstrapper)? I think IMHO this would be a somewhat cleaner way to write it:

// /server/middleware/react-application/index.js

  const App = () => (
    <AsyncComponentProvider asyncContext={asyncComponentsContext}>
      <StaticRouter location={request.url} context={reactRouterContext}>
        <DemoApp />
      </StaticRouter>
    </AsyncComponentProvider>
  );

  // Pass our app into the react-async-component helper so that any async
  // components are resolved for the render.
  asyncBootstrapper(App()).then(() => {

    // Generate the html response.
    const html = renderToNodeStream(
      <ServerHTML
        nonce={nonce}
        helmet={Helmet.rewind()}
        asyncComponentsState={asyncComponentsContext.getState()}
      >
        <App />
      </ServerHTML>,
    );

I could push that if you like! I confess I'm not totally clear on how Asyncbootstrapper works (I understand the basics), so it's quite possible I could be wrong. In any case, I just pushed the changes you suggested with the latest Next merged in.

[diondirza]

Ah sorry, you right. It is better to write it like that. I thought asyncBootstrapper was impure function that mutates App component state. I think the codes on your last comment much cleaner and makes more sense.

[diondirza]

If you look at the AB source code, it only triggers asyncBootstrap function inside our component.

[oyeanuj]

@unleashit This is great, thank you for the PR. This will now enable us to get streaming styles in the Styled-Component branch as well!

[williamoliveira]

Do redirects still works?

[unleashit]

No problem at all. Just made the change. To be honest, I think it might be a good idea to do some further testing this with actual projects before committing. I think it's fine for the base project, but I wonder if some people might have issues if they use libraries that aren't compatible with streaming. It may be that recent React releases improved things, because at least a couple months ago Helmet didn't work, but now it does (at least with the base project).

@oyeanuj even though I hate styled components with a passion (I'm a fan of CSS modules for encapsulation which keeps the ability to simple copy/paste the plethora of community styles all over the internet and past projects), that's great if true!

[unleashit]

@williamoliveira do you mean server or client side/RR?

[williamoliveira]

Server. Does the asyncBootstrapper pass populates reactRouterContext?

[unleashit]

Looks like it's fine:

EDIT: also just did a little test to confirm RR context was avail on the server and it was. I do agree though its worth questioning and testing...

[williamoliveira]

I mean this but never mind, it doesn't works the way I thought it did, this switch is not being used by the looks of it 404 page sets a missed = true attribute and not status = 404 and <Redirect /> doesn't sets status alone, you need to set it yourself

[unleashit]

Ok, now I see what you mean. Yeah, I wondered about that myself. I guess someone put the switch in in case you want to let the react middleware manage these things. Personally, I let my 404s get handled at the end of my main switch so they're templated. I put my 301/302s as a middleware in server.js above app.get(*).

[oyeanuj]

because at least a couple months ago Helmet didn't work, but now it does (at least with the base project).

@unleashit AFAIU, for streaming SSR with React 16, this fork of React-Helmet is recommended: https://github.com/NYTimes/react-helmet-async

Have you had a look at it?

[unleashit]

Just spent a few minutes learning about the problem with Helmet and it looks like it still exists. Even though it seems to be fine in a dev situation, race conditions can sometimes happen.

Looks like there are at least a couple helmet forks out there that try to fix streaming: https://github.com/NYTimes/react-helmet-async and https://github.com/kouhin/react-safety-helmet. Not sure if its ideal to rely on one of these, but IMO both getting streaming to work and what Helmet does are super important.

If you guys wanted me to implement one of them, I'd be willing to take a shot over the next few days.

[oyeanuj]

@unleashit That would be awesome, since fixing Helmet would become critical for SEO purposes.

Also, you probably already came across this, but wanted to put it out there as an implementation of renderNodeStream with caching: https://zeit.co/blog/streaming-server-rendering-at-spectrum#caching-html-in-node.js , especially incase anyone wants to enhance their streaming experience.

[unleashit]

Hadn't seen that. Cool trick with the transform stream. But I wonder if we should be adding a page caching strategy to this repo? IMO, that gets complicated and might make things harder for people wanting to do things a different way. Also from my understanding, streaming React is going to be most useful for when you "aren't" taking the trouble to set up a caching system since after the first hit, the next users are getting the cache anyway. Personally, I've been wondering (but not necessarily for this project) if its worth the trouble to integrate a site generator for static pages while keeping server rendering for authed/dynamic pages.

[oyeanuj]

@unleashit I see the benefit for caching strategy when you don't want to go fetch a lot of data for pages which are likely to be static (and especially for search engines).

[unleashit]

Totally agree, but would it be a good idea to add an opinionated one here? There are many other ways to set it up depending on the situation. Ex: Nginx, Varnish, or like I was describing to precompile with a static site generator like https://github.com/markdalgleish/static-site-generator-webpack-plugin and serve from a CDN. If we did set up a system managed by the app, then we'd also have to decide where to keep the cache. Keeping it in memory is a bad idea esp if you have more than one process, so you'd need a DB or cache like Redis.... another area where everyone has a different opinion/need.

[oyeanuj]

@unleashit Agree with you that it might be too opinionated for this kit.

[unleashit]

Just swapped out react-helmet for react-helmet-async. It snapped in pretty easily. The main difference is RHA adds a context provider to an initial rendering of the tree (which RU happens to be doing with Async Bootstrapper), then stores React Helmet state in a variable. The metadata is then passed to the main render and everything works as before. The only other thing that had to be changed was the import names in the App from react-helmet to react-helmet-async and the client App had also be wrapped in the provider.

This seems like a good way to go as far as I can tell. I imagine New York Times will maintain the fork for awhile at least/until the main React Helmet fixes decides to support streaming.

[oyeanuj]

@unleashit This is great, thank you for the work! Just curious to hear your take on the need for react-safety-helmet since I know you were mentioning it earlier.

[unleashit]

No problem, it was really nothing compared to what others have done! React-safety-helmet is just another react-helmet fork that tries to solve the side effects issue.

TBH I picked the NYT one for this because it seems more likely that NYT is going to keep it maintained. But also at a glance, it seems like the idea of React Safety Helmet to pipe renderToNodeStream directly into another stream which simply waits, then adds Helmet before sending the stream to the browser. So just as blocking as what's going on here. NYT also needs a first parse which this repo is already setup to do with Async Boostrapper, etc. I'm no expert on this stuff, but it's working pretty good I think ;-)

[oyeanuj]

@unleashit @diondirza Is this good to merge?

[unleashit]

Hi @oyeanuj, it works fine with the base install but feel like it wouldn't be a bad idea for it to be tested out on a bigger app that has more going on (async calls, complex nested components that generate a lot of markup, etc.).

Maybe if @ctrlplusb could give his opinion? I haven't taken a deep dive into what his async component and bootstrapper do so maybe he might know if there are any edge cases we could miss other than React Helmet (which is patched here). I'm not currently using this starter kit and haven't in almost a year (partly because I've been working on existing apps), but I would be more than happy to make any updates needed including update react-helmet-async.

For the record, I do love this starter and it would be great to see it come back to life. I'm a fan of this over frameworks like Next.js or RCA because its much harder to break out of their opinionated choices (proprietary router, css in js by default, no server rendering for RAC, etc.).

[oyeanuj]

@unleashit thanks, i'll give it a shot. FWIW, I feel the same way about starter kits like these over those frameworks.

@diondirza Have you had a chance to test this?

[diondirza]

Currently using the same setup as this PR in prod. This is good to be merged.

[oyeanuj]

@unleashit @diondirza I'm seeing issues with react-helmet-async not updating the meta tags on the server - did either of you see that at any point?

[unleashit]

It seemed fine for me, but I haven't spent any time on this for a long time since this project appears to be dead. I'm also using Typescript now and have a different setup.

The one part I was unsure of was RHA eliminates all possibility of a race condition or not. So I think it would be a good idea to create a test (maybe with headless chrome) that sends a lot of concurrent requests and checks for the right head tags.

I noticed there's an update to RHA, so that's probably worth trying.