Skip to content
This repository has been archived by the owner on Aug 5, 2020. It is now read-only.

Set up ability to inline CSS into server response. #1194

Merged
merged 7 commits into from
May 23, 2018

Conversation

threehams
Copy link
Collaborator

Save a round trip for CSS. Makes a big difference when initial load times are more important than return visits.

Save a round trip. Makes a big difference.
path.join(process.cwd(), 'build', stylesHref),
);
styleTags.push(
<style dangerouslySetInnerHTML={{ __html: contents.toString() }} />,
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be mistaken, but I think recent react-helmet allows styles with text as children.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, but we're really low-level here (initial server render), and Helmet isn't really designed for order-dependent stuff like styles.

};

Object.keys(assetsByChunk).forEach((name: string) => {
// The second asset is usually a source map
const jsAsset: string = getAssets(name, 'js')[0];
const jsAsset = getAssets(name, 'js')[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

why no more flow typing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

commented a bit late: most of the Flow types in this file are redundant and already inferred. I improved typing overall by getting rid of some Object ones.

@@ -22,17 +13,17 @@ const chunkInfoFilePath = (
return path.join(webpackConfiguration.output.path, chunkInfoFilename);
};

const getChunksInfoBody = (json: Object, publicPath: string): ChunksInfo => {
const assetsByChunk: Object = json.assetsByChunkName;
const getChunksInfoBody = (stats: Object, publicPath: string): ChunksInfo => {
Copy link
Collaborator Author

@threehams threehams May 22, 2018

Choose a reason for hiding this comment

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

ChunksPlugin is from universal-webpack and was copy/pasted in here a while ago. It tracks JS + CSS chunks and writes the information to a file. I modified it to include the original filename in addition to the URL.


const React = require('react');
const path = require('path');
const fs = require('fs');
// $FlowIgnore promisify is not available in this version of Flow
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgrading Flow will flag this as an error. We can remove it at that time.

const localPath = path.join(process.cwd(), 'build', 'assets', name);
const contents = await readFileAsync(localPath);

cache[name] = contents.toString();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will definitely increase server memory usage, but will not cause garbage collection calls. I'll monitor it after release.

Copy link
Collaborator

@jacobmoretti jacobmoretti May 22, 2018

Choose a reason for hiding this comment

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

Curious how you''ll monitor this. I haven't looked into it too much on NR but do we get pretty good visibility into the health of our node instances through NR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can leverage datadog for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Today I learned that datadog exists.

let key: number = 0;
const entryPointName: string = filterEntryName(entryPoint);
) {
const styleTags = [];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed a lot of unnecessary Flow typing from this file. All of this is inferred.

@threehams threehams merged commit 619e8cc into develop May 23, 2018
@threehams threehams deleted the feature/inline-all-css branch May 23, 2018 16:51
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.

5 participants