Skip to content
This repository has been archived by the owner on Jun 3, 2019. It is now read-only.

renderToNodeStream the full app without renderToString #564

Open
wants to merge 10 commits into
base: next
Choose a base branch
from

Conversation

unleashit
Copy link

@unleashit unleashit commented Mar 14, 2018

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).

@unleashit unleashit changed the title renderToNodeStream the full app without renderToStream renderToNodeStream the full app without renderToString Mar 14, 2018
appBodyString={reactAppString}
/>
>
<div id="app">{children}</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be {children} only

@@ -137,12 +138,12 @@ ServerHTML.propTypes = {
// eslint-disable-next-line react/forbid-prop-types
helmet: PropTypes.object,
nonce: PropTypes.string,
reactAppString: PropTypes.string,
children: PropTypes.object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be PropTypes.node

};

ServerHTML.defaultProps = {
asyncComponentsState: undefined,
helmet: undefined,
nonce: undefined,
reactAppString: undefined,
children: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React expect null element instead of undefined right?

// Generate the html response.
const html = renderToNodeStream(
<ServerHTML
reactAppString={appString}
// reactAppString={appString}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should preserve this code as comment

@@ -62,19 +58,21 @@ export default function reactApplicationMiddleware(request, response) {
</AsyncComponentProvider>
);

const App = () => app;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

place this below asyncBootstrapper, because async bootstrapper run all function to preloaded data in with react-async-component, on line 68 that you deleted add this code instead
const ResolvedApp = () => app;


return (
<html {...htmlAttributes}>
<head>{headerElements}</head>
<body>
<div id="app" dangerouslySetInnerHTML={{ __html: appBodyString }} />
{children}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should remain with this
<div id="app">{children}<div>
to have consistent element between server and client render

@@ -26,14 +26,14 @@ HTML.propTypes = {
htmlAttributes: PropTypes.object,
headerElements: PropTypes.node,
bodyElements: PropTypes.node,
appBodyString: PropTypes.string,
children: PropTypes.object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be node type

};

HTML.defaultProps = {
htmlAttributes: null,
headerElements: null,
bodyElements: null,
appBodyString: '',
children: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be null as default

@diondirza
Copy link
Contributor

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

@unleashit
Copy link
Author

unleashit commented Mar 17, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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

@oyeanuj
Copy link
Contributor

oyeanuj commented Mar 17, 2018

@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
Copy link

Do redirects still works?

@unleashit
Copy link
Author

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
Copy link
Author

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

@williamoliveira
Copy link

Server.
Does the asyncBootstrapper pass populates reactRouterContext?

@unleashit
Copy link
Author

unleashit commented Mar 17, 2018

Looks like it's fine:

rrcontext

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
Copy link

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
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Mar 18, 2018

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
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Mar 18, 2018

@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
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Mar 25, 2018

@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
Copy link
Author

unleashit commented Mar 26, 2018

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
Copy link
Contributor

oyeanuj commented Mar 26, 2018

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

@unleashit
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Mar 29, 2018

@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
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Jul 24, 2018

@unleashit @diondirza Is this good to merge?

@unleashit
Copy link
Author

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
Copy link
Contributor

oyeanuj commented Jul 25, 2018

@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
Copy link
Contributor

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

@oyeanuj
Copy link
Contributor

oyeanuj commented Dec 29, 2018

@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
Copy link
Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants