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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved server rendering #9343

Merged
merged 14 commits into from Nov 23, 2017
Merged

Improved server rendering #9343

merged 14 commits into from Nov 23, 2017

Conversation

@jbaxleyiii
Copy link
Contributor

@jbaxleyiii jbaxleyiii commented Nov 10, 2017

馃憢 With the release of React 16, a new way to render on the server is streaming the rendering of the react portion of your app to the client. This PR adjusts Meteor's internal markup generation for the server to move to a stream first API. With this change, you can use a string (renderToString) or a stream (renderToNodeStream) when adding content into an id.

In initial testing, it results in a 40% reduction of TTFB 馃尞 (Time To First Byte [of taco]).

Todo

  • ensure support for multiple id based additions of streams of strings
  • update tests
  • add support for redirects
  • add support for changing status code
  • provide access to headers
  • write tests for new features

Wanted to get this up early enough for review though!

This PR address setting status codes including for SEO, improved alignment with react features, setting headers like eTag, and accessing headers and cookies

@jbaxleyiii jbaxleyiii self-assigned this Nov 10, 2017
@jbaxleyiii jbaxleyiii requested review from benjamn and abernix Nov 10, 2017
@jbaxleyiii jbaxleyiii force-pushed the jbaxleyiii/streaming-server-render branch to 8a43f71 Nov 10, 2017
@benjamn benjamn added this to the Release 1.6.1 milestone Nov 10, 2017

#### Meteor Accounts
Meteor's authentication system uses cookies to store the login token for
an authenticated user. To get access to this, you can use the `getCookies` method

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

I wish this statement was true, since it would allow server-side rendering to work much better with with the Meteor accounts-* packages. Unfortunately, the login token is stored in the browser's localStorage, which means it can't be sent in a cookie, and must be read by client JavaScript after the response is sent back.




#### React 16 renderToNodeStream

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

Let's put markdown backticks around this function?


// server only methods
setStatusCode() {
console.error(isoError("setStatusCode"));

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

Could just move the console.error call into isoError.


const end = this.closeTemplate(data);

return { start, stream, end }

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

Can't we just include the start and end chunks in the stream, so we don't have to return multiple things here?

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Nov 15, 2017
Author Contributor

haha duh, wow should've done that from the start (see what I did there 馃槈)

stream.on("end", () => {
res.write(end);
res.end();
})

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

For example, if start and end were part of the stream, this code could just be

stream.pipe(res, { end: true });
@@ -8,6 +8,24 @@ var hash = crypto.createHash('sha1');
hash.update(additionalScript);
var additionalScriptPathname = hash.digest('hex') + ".js";

// convert a stream to a string via promise
function toString(stream) {

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

Alright, this code is duplicated three times now. Let's find a better home for it!

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

If you want to just use an npm package, concat-stream looks good to me.

var string = ''
stream.on('data', function(data) {
string += data.toString();
});

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

I think it might be better if we appended the data buffers to an array and then used Buffer.concat(chunks).toString("utf8") at the end, since a chunk boundary could theoretically split a multi-byte unicode character.

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

Example of other code that we use for a similar purpose.

@@ -3,6 +3,10 @@ Package.describe({
version: '1.3.1'
});

Npm.depends({
"combine-streams": "1.0.0"

This comment has been minimized.

@benjamn

benjamn Nov 12, 2017
Member

I worry that the combine-streams package is not very heavily developed/starred/downloaded. In particular, it doesn't seem to pay any attention to the string encoding of the written chunks.

After a bit of digging, this combined-stream2 package looks a bit more mature.

@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 13, 2017

@benjamn thanks for the comments! I'll work to address them ASAP!

@SachaG
Copy link
Contributor

@SachaG SachaG commented Nov 14, 2017

Just wanted to say it's awesome to see things moving so fast! Hope to be able to integrate this in Vulcan soon :)

@hwillson
Copy link
Member

@hwillson hwillson commented Nov 15, 2017

@jbaxleyiii Since hack week is over and you're back busy on all things Apollo, just say the word and I'll jump in to finish this PR up. I'm super excited to get these changes out - thanks for working on this!

@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 15, 2017

@hwillson if you have time that would be amazing! There are a number of apollo client 2.0 upgrade blockers that I need to put my full focus on.

I'm going to try to get all of the changes done by Friday but if not then if you and @benjamn can take it over it would be so great to see this one make it through.

@benjamn does that timeline work? I want to make sure its ready for PR club next week

@abernix
Copy link
Member

@abernix abernix commented Nov 15, 2017

Next week is great, @jbaxleyiii. Thanks for your work on this! 馃槃

@jbaxleyiii jbaxleyiii force-pushed the jbaxleyiii/streaming-server-render branch from 8a43f71 to 8e6a6c6 Nov 19, 2017
@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 19, 2017

@hwillson @benjamn @abernix everything should be addressed with the latest commit! Looking forward to seeing this in 1.6 馃槏

@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 19, 2017

@benjamn of careful note is the removal of the boilerplate memoization (done on a separate commit to make it easy to revert)

I think I'm probably misunderstanding its goals, but given the potential dynamic nature of generating markup within an app, memoization of request => markup seems like a bad idea?

@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 19, 2017

@hwillson one thing this is still missing is tests for redirection, setting status codes, and getting / setting headers. I don't think I'll have time to write those sadly :(

Copy link
Member

@benjamn benjamn left a comment

This is looking great! Just a few nits to pick. And of course, if you run out of time for working on this, just let @abernix and me know, and we can finish it up.

@@ -50,5 +52,7 @@ export function generateHTMLForArch(arch) {
},
});

return boilerplate.toHTML();

return await toString(boilerplate.toHTML());

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

No need to explicitly await when returning a value that could be a Promise from an async function.

/<script[^<>]*>[^<>]*__meteor_runtime_config__ =.*decodeURIComponent\(config123\)/
);
}
run().then(onComplete);

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

I think you need to handle the error case here, too, so the test doesn't time out if there's an error.

}
);
const start = async () => {
const html = await generateHTMLForArch('web.browser');

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

Rather than using an async function here, I would just do

generateHTMLForArch("web.browser").then(html => {
  Tinytest.add(...);
  ...
});

I'm not entirely sure what happens if the tests finish while there's still a pending Promise, so you might want to capture the resulting promise and add an additional

Tinytest.addAsync("everything ok", function (test, onComplete) {
  promise.then(
    () => onComplete,
    error => test.fail(error)
  );
});

at the end of the file.

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

We should really just improve Tinytest to handle Promises better, honestly, but that's a mission for a different PR.

const end = this.closeTemplate(data);
const response = stream.create();

response.append(Buffer.from(start))

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

Let's be explicit about passing "utf8" as the encoding type as the second argument to Buffer.from.


response.append(Buffer.from(start))
if (body) {
response.append(typeof body === "string" ? Buffer.from(body) : body)

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

What else can body be? Let's enforce our assumptions here using some combination of typeof body === "string", Buffer.isBuffer, and typeof body.read === "function" (if that covers all the cases).

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

The internet tells me body instanceof ReadableStream isn't always safe, since folks can implement their own stream-like objects, which is why I suggested the dynamic duck test that body.read is a function. Maybe extract this logic into a helper function so you can do appendToStream(response, body)?

@@ -1,13 +1,15 @@
var url = Npm.require("url");
var crypto = Npm.require("crypto");
var http = Npm.require("http");
var toString = Npm.require("stream-to-string");

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

streamToString please

@@ -1,13 +1,15 @@
var url = Npm.require("url");

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

Let's just switch these tests to depending on ecmascript so we can use imports (or at least require) here.

clientDir: "/"
};
Tinytest.addAsync("webapp - additional static javascript", function (test, onComplete) {
const run = async () => {

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

I think you can just make the whole test function async, and call onComplete after the last await. I don't think Tinytest will do anything smart with the resulting Promise object, but that's fine.

}
if (newHeaders) {
headers = {...headers, ...newHeaders };
}

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

Let's just use Object.assign(headers, newHeaders) here.

res.write(boilerplate);
res.end();
}, error => {
stream.pipe(res)

This comment has been minimized.

@benjamn

benjamn Nov 20, 2017
Member

Does this need to be stream.pipe(res, { end: true })?

@jbaxleyiii
Copy link
Contributor Author

@jbaxleyiii jbaxleyiii commented Nov 21, 2017

@benjamn thanks for the further notes! I'll try to get them done asap!

@benjamn benjamn self-assigned this Nov 22, 2017
@benjamn benjamn force-pushed the jbaxleyiii/streaming-server-render branch from e300a33 Nov 23, 2017
@benjamn benjamn changed the base branch from devel to modernize-tinytest Nov 23, 2017
@benjamn benjamn force-pushed the modernize-tinytest branch to 45e396d Nov 23, 2017
jbaxleyiii added 5 commits Nov 10, 2017
jbaxleyiii and others added 9 commits Nov 10, 2017
@benjamn benjamn force-pushed the jbaxleyiii/streaming-server-render branch to 8009351 Nov 23, 2017
@benjamn
Copy link
Member

@benjamn benjamn commented Nov 23, 2017

@jbaxleyiii Thanks for all your work on this. Since you're busy now, I went ahead and addressed my latest feedback, and in the process I was inspired to improve Tinytest.addAsync to enable the kind of tests you wanted to write (see #9409). 馃帀

@benjamn benjamn changed the base branch from modernize-tinytest to devel Nov 23, 2017
@benjamn benjamn merged commit f3c6460 into devel Nov 23, 2017
13 checks passed
13 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
benjamn added a commit that referenced this pull request Nov 25, 2017
@jbaxleyiii jbaxleyiii deleted the jbaxleyiii/streaming-server-render branch Nov 27, 2017
benjamn added a commit that referenced this pull request Jan 9, 2018
PR #9343 changed the return type of Boilerplate#toHTML from String to
Stream, which is likely to break existing code that expects a string.

In order to make the change in return type more obvious, I have renamed
the method to toHTMLStream, and I have attempted to update all call sites
appropriately. However, because this change comes in the release candidate
phase of Meteor 1.6.1 testing, it seemed important to preserve the
string-returning behavior of toHTML, with a deprecation notice.

Unless third-party code is using the Boilerplate class directly, I don't
think the toHTML method will ever be called, and we can remove it in
Meteor 1.6.2.

Thanks to @macrozone for tracking this problem down.

Fixes #9521.
@williamli
Copy link

@williamli williamli commented Feb 8, 2018

I tried to replace renderToString with renderToNodeStream in my React SSR.
The browser ended up stuck waiting for the stream to start (stuck in loading, but nothing ever arrived).

I have Helmet, styled-components, and node-cache on the server side logic.
After removing all of them, leaving just the bare essentials

sink.renderIntoElementById("react-app", renderToNodeStream(<App target="server" location={sink.request.url.path} visitorGeoDetails={visitorGeoDetails} setPreloadedContent={setPreloadedContent} />));

The problem still remains.

Is there any other thing I need to do when switching over to renderToNodeStream ?

PS: I see the server ran through the React Components (and if there were any warnings from propTypes, they also showed up in the server terminal console log when a browser try to access the site).

@evolross
Copy link
Contributor

@evolross evolross commented Oct 11, 2019

It's been a minute but the removal of the boilerplate memoization is causing setBundledJsCssUrlRewriteHook to fire a lot (and apparently infinitely looping in production on Galaxy). Wanting to make sure there's no negative consequence. #10731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can鈥檛 perform that action at this time.