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(gatsby): Extract non-css-in-js css and add add to <head> when SSRing in dev #28471

Merged
merged 44 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
ee315ed
feature(gatsby): Pause dev-ssr watching between page loads to avoid s…
KyleAMathews Dec 1, 2020
b94ced1
update snapshot
KyleAMathews Dec 1, 2020
0228936
Don't double-resolve + add activity for building the SSR bundle
KyleAMathews Dec 1, 2020
135e960
Merge remote-tracking branch 'origin/master' into lazy-compile
KyleAMathews Dec 1, 2020
b96c520
Add timeout for tests to ensure that dev server has time to bundle SS…
KyleAMathews Dec 1, 2020
fee4f5a
Merge remote-tracking branch 'origin/master' into lazy-compile
KyleAMathews Dec 1, 2020
36e7f9e
feature(gatsby): Extract and add CSS when SSRing in dev
KyleAMathews Dec 3, 2020
3357d79
Remove commented out code
KyleAMathews Dec 3, 2020
b3bb3b4
get tests passing
KyleAMathews Dec 4, 2020
64bc7f9
WIP
KyleAMathews Dec 4, 2020
8e5f6f0
Got hot-reloading working w/ mini-css-extract-plugin
KyleAMathews Dec 4, 2020
468a286
remove mistakenly added file
KyleAMathews Dec 4, 2020
410424c
remove change to yarn.lock
KyleAMathews Dec 4, 2020
d27291f
revert other mistakenly added files
KyleAMathews Dec 4, 2020
68b32ad
Add an async module to test against
KyleAMathews Dec 4, 2020
e6b4c39
fix async module
KyleAMathews Dec 4, 2020
9ecb314
Add postcss/tailwind
KyleAMathews Dec 4, 2020
84a4f2c
write webpack config for easy comparisons
KyleAMathews Dec 7, 2020
3e8fa67
that was a lot easier than I thought — just set hmr:true for non-prod…
KyleAMathews Dec 7, 2020
baf7a65
cleanups
KyleAMathews Dec 7, 2020
31354f2
Don't need this since we're using <link> not <style>
KyleAMathews Dec 7, 2020
92e4656
pass in port
KyleAMathews Dec 7, 2020
5a35154
remove dev css from test comparisons
KyleAMathews Dec 7, 2020
d4ae483
Update snapshots + add tailwind
KyleAMathews Dec 7, 2020
d4d5340
cleanups
KyleAMathews Dec 7, 2020
59148ae
remove discarded changes
KyleAMathews Dec 8, 2020
08ee647
Move changes behind flag
KyleAMathews Dec 8, 2020
c1eeff7
Undo unnecesary changes
KyleAMathews Dec 8, 2020
1f9fbdc
Update tests for signature change
KyleAMathews Dec 8, 2020
1125d6c
Move more code behind the flag
KyleAMathews Dec 8, 2020
51bbd3b
Merge branch 'lazy-compile' into mini-css-dev-ssr
KyleAMathews Dec 8, 2020
4fbb812
dynamically set absolute URL for css files so works wherever it's hosted
KyleAMathews Dec 8, 2020
e563bda
start relative than make absolute
KyleAMathews Dec 8, 2020
d59c6e2
Remove now unused port
KyleAMathews Dec 21, 2020
627239c
Remove changes from https://github.com/gatsbyjs/gatsby/pull/28394/
KyleAMathews Dec 21, 2020
1afed99
use @pieh's suggested refactor in https://github.com/gatsbyjs/gatsby/…
KyleAMathews Dec 21, 2020
3c088a5
pass naming options for extractText in via options
KyleAMathews Dec 21, 2020
139efda
Update packages/gatsby/src/utils/webpack.config.js
KyleAMathews Dec 22, 2020
6d3a64b
Update snapshot
KyleAMathews Dec 22, 2020
b1506c0
Stop Jest from chocking on import of css
KyleAMathews Dec 22, 2020
9c34498
turned out we didn't need this
KyleAMathews Dec 22, 2020
0b78528
test(ssr): ignore src/test file (those are not tests)
pieh Jan 4, 2021
a22361f
test(ssr): update snapshot after removing inline script modyfing href
pieh Jan 4, 2021
f01a5fd
Merge remote-tracking branch 'origin/master' into mini-css-dev-ssr
pieh Jan 4, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integration-tests/ssr/__mocks__/styleMock.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = {}
2 changes: 1 addition & 1 deletion integration-tests/ssr/__tests__/__snapshots__/ssr.js.snap
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`SSR is run for a page when it is requested 1`] = `"<!DOCTYPE html><html><head><meta charSet=\\"utf-8\\"/><meta http-equiv=\\"x-ua-compatible\\" content=\\"ie=edge\\"/><meta name=\\"viewport\\" content=\\"width=device-width, initial-scale=1, shrink-to-fit=no\\"/><meta name=\\"note\\" content=\\"environment=development\\"/><script src=\\"/socket.io/socket.io.js\\"></script></head><body><div id=\\"___gatsby\\"><div style=\\"outline:none\\" tabindex=\\"-1\\" id=\\"gatsby-focus-wrapper\\"><div>Hello world</div></div><div id=\\"gatsby-announcer\\" style=\\"position:absolute;top:0;width:1px;height:1px;padding:0;overflow:hidden;clip:rect(0, 0, 0, 0);white-space:nowrap;border:0\\" aria-live=\\"assertive\\" aria-atomic=\\"true\\"></div></div><script src=\\"/polyfill.js\\" nomodule=\\"\\"></script><script src=\\"/commons.js\\"></script></body></html>"`;
exports[`SSR is run for a page when it is requested 1`] = `"<!DOCTYPE html><html><head><meta charSet=\\"utf-8\\"/><meta http-equiv=\\"x-ua-compatible\\" content=\\"ie=edge\\"/><meta name=\\"viewport\\" content=\\"width=device-width, initial-scale=1, shrink-to-fit=no\\"/><link data-identity=\\"gatsby-dev-css\\" rel=\\"stylesheet\\" type=\\"text/css\\" href=\\"/commons.css\\"/><meta name=\\"note\\" content=\\"environment=development\\"/><script src=\\"/socket.io/socket.io.js\\"></script></head><body><div id=\\"___gatsby\\"><div style=\\"outline:none\\" tabindex=\\"-1\\" id=\\"gatsby-focus-wrapper\\"><div><h1 class=\\"hi\\">Hello world</h1></div></div><div id=\\"gatsby-announcer\\" style=\\"position:absolute;top:0;width:1px;height:1px;padding:0;overflow:hidden;clip:rect(0, 0, 0, 0);white-space:nowrap;border:0\\" aria-live=\\"assertive\\" aria-atomic=\\"true\\"></div></div><script src=\\"/polyfill.js\\" nomodule=\\"\\"></script><script src=\\"/commons.js\\"></script></body></html>"`;

exports[`SSR it generates an error page correctly 1`] = `
"<head>
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/ssr/__tests__/ssr.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@ describe(`SSR`, () => {
}, 400)
}, 400)
})
})
}, 15000)
})
1 change: 1 addition & 0 deletions integration-tests/ssr/gatsby-browser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "./sample.css"
2 changes: 1 addition & 1 deletion integration-tests/ssr/gatsby-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ module.exports = {
github: `sidharthachatterjee`,
moreInfo: `Sid is amazing`,
},
plugins: [],
plugins: ["gatsby-plugin-postcss"],
}
13 changes: 12 additions & 1 deletion integration-tests/ssr/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
module.exports = {
testPathIgnorePatterns: [`/node_modules/`, `__tests__/fixtures`, `.cache`],
testPathIgnorePatterns: [
`/node_modules/`,
`__tests__/fixtures`,
`.cache`,
`src/test`,
],
transform: {
"^.+\\.[jt]sx?$": `<rootDir>../../jest-transformer.js`,
},
moduleNameMapper: {
"\\.(css)$": `<rootDir>/__mocks__/styleMock.js`,
},
}
4 changes: 3 additions & 1 deletion integration-tests/ssr/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
},
"dependencies": {
"gatsby": "^2.27.0",
"gatsby-plugin-postcss": "^3.3.0",
"react": "^16.12.0",
"react-dom": "^16.12.0"
"react-dom": "^16.12.0",
"tailwindcss": "1"
},
"devDependencies": {
"cross-env": "^5.0.2",
Expand Down
3 changes: 3 additions & 0 deletions integration-tests/ssr/postcss.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
plugins: [require("tailwindcss"), require("autoprefixer")],
}
6 changes: 6 additions & 0 deletions integration-tests/ssr/sample.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
body {
background: tomato;
color: black;
font-style: italic;
font-weight: 400;
}
7 changes: 6 additions & 1 deletion integration-tests/ssr/src/pages/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from "react"
import { useStaticQuery, graphql } from "gatsby"
const lazyImport = import(`../test`)
pieh marked this conversation as resolved.
Show resolved Hide resolved

export default function Inline() {
const { site } = useStaticQuery(graphql`
Expand All @@ -11,5 +12,9 @@ export default function Inline() {
}
}
`)
return <div>{site.siteMetadata.title}</div>
return (
<div>
<h1 className="hi">{site.siteMetadata.title}</h1>
</div>
)
}
4 changes: 4 additions & 0 deletions integration-tests/ssr/src/pages/usingtailwind.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import React from "react"
import "../styles/tailwind.css"
pieh marked this conversation as resolved.
Show resolved Hide resolved

export default () => <h1 className="text-3xl">This is a 3xl text</h1>
5 changes: 5 additions & 0 deletions integration-tests/ssr/src/styles/tailwind.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@tailwind base;

@tailwind components;

@tailwind utilities;
3 changes: 3 additions & 0 deletions integration-tests/ssr/src/test.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.hi {
color: blue;
}
1 change: 1 addition & 0 deletions integration-tests/ssr/src/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "./test.css"
10 changes: 10 additions & 0 deletions integration-tests/ssr/tailwind.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module.exports = {
purge: [
'./src/**/*.js',
],
theme: {
extend: {}
},
variants: {},
plugins: []
}
4 changes: 3 additions & 1 deletion integration-tests/ssr/test-output.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@
const $ = cheerio.load(htmlStr)
// There are many script tag differences
$(`script`).remove()
// Only added in production. Dev uses css-loader
// Only added in production
$(`#gatsby-global-css`).remove()
// Only added in development
$(`link[data-identity='gatsby-dev-css']`).remove()
// Only in prod
$(`link[rel="preload"]`).remove()
// Only in prod
Expand Down
22 changes: 20 additions & 2 deletions packages/gatsby/cache-dir/__tests__/static-entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from "react"
import fs from "fs"
const { join } = require(`path`)

import ssrDevelopStaticEntry from "../ssr-develop-static-entry"
import developStaticEntry from "../develop-static-entry"

jest.mock(`fs`, () => {
Expand All @@ -22,7 +21,7 @@ jest.mock(
() => {
return {
ssrComponents: {
"page-component---src-pages-test-js": () => null,
"page-component---src-pages-about-js": () => null,
},
}
},
Expand Down Expand Up @@ -151,8 +150,27 @@ const fakeComponentsPluginFactory = type => {
}
}

const SSR_DEV_MOCK_FILE_INFO = {
[`${process.cwd()}/public/webpack.stats.json`]: `{}`,
pieh marked this conversation as resolved.
Show resolved Hide resolved
[join(
process.cwd(),
`/public/page-data/about/page-data.json`
)]: JSON.stringify({
componentChunkName: `page-component---src-pages-about-js`,
path: `/about/`,
webpackCompilationHash: `1234567890abcdef1234`,
staticQueryHashes: [],
}),
[join(process.cwd(), `/public/page-data/app-data.json`)]: JSON.stringify({
webpackCompilationHash: `1234567890abcdef1234`,
}),
}

describe(`develop-static-entry`, () => {
let ssrDevelopStaticEntry
beforeEach(() => {
fs.readFileSync.mockImplementation(file => SSR_DEV_MOCK_FILE_INFO[file])
ssrDevelopStaticEntry = require(`../ssr-develop-static-entry`).default
global.__PATH_PREFIX__ = ``
global.__BASE_PATH__ = ``
global.__ASSET_PREFIX__ = ``
Expand Down
67 changes: 65 additions & 2 deletions packages/gatsby/cache-dir/ssr-develop-static-entry.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from "react"
import fs from "fs"
import { renderToString, renderToStaticMarkup } from "react-dom/server"
import { merge } from "lodash"
import { get, merge, isObject, flatten, uniqBy, concat } from "lodash"
import { join } from "path"
import apiRunner from "./api-runner-ssr"
import { grabMatchParams } from "./find-path"
Expand All @@ -20,6 +20,10 @@ const testRequireError = (moduleName, err) => {
return regex.test(firstLine)
}

const stats = JSON.parse(
fs.readFileSync(`${process.cwd()}/public/webpack.stats.json`, `utf-8`)
)

let Html
try {
Html = require(`../src/html`)
Expand Down Expand Up @@ -111,7 +115,66 @@ export default (pagePath, isClientOnlyPage, callback) => {

const pageData = getPageData(pagePath)

const componentChunkName = pageData?.componentChunkName
const { componentChunkName, staticQueryHashes = [] } = pageData

let scriptsAndStyles = flatten(
[`commons`].map(chunkKey => {
const fetchKey = `assetsByChunkName[${chunkKey}]`

let chunks = get(stats, fetchKey)
const namedChunkGroups = get(stats, `namedChunkGroups`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you do _.get(stats, `namedChunkGroups`) over stats.namedChunkGroups? I don't understand this. Even for _.get(stats, fetchKey), I think stats[fetchKey] is better.

If the object may not exist, we should use stats?.fetchKey. For a default we should use stats?.fetchKey ?? defValue. Either way, I think there are very few cases where I could see _.get be preferred at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy/pasted this from static-entry.js which was written a long long time ago pre fancy-dancy stuff like ?. Is there a reason to refactor other than preference? I'd rather keep the implementation of the two the same.

Copy link
Contributor

@pvdz pvdz Dec 9, 2020

Choose a reason for hiding this comment

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

The main reason is technical debt. I don't think this is a preference but rather about redundant outdated code.

My suggestion; if there's a strong desire to keep them both, is to share this code somehow, rather than to copy/paste. It may be copy/paste now, but in some future somebody will look at this file and not realize that this is the case, just like I didn't know when I was reviewing this.

If nothing else, and there's a desire to keep them in sync, there ought to be a comment or notice about this somewhere in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extracting out to a common function is a great idea — I hadn't done that earlier as things were in flux but let's do that & then we can refactor/modernize the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do this separately, extracting to common util can have some impact on regular ssr, and we want to backport this for tomorrow's release so it's safer to ship this as-is (as everything here touches pretty much things behind the flag) and we can make common util (and also address outdated code / unneeded use of lodash too)


if (!chunks) {
return null
}

chunks = chunks.map(chunk => {
if (chunk === `/`) {
return null
}
return { rel: `preload`, name: chunk }
})

namedChunkGroups[chunkKey].assets.forEach(asset =>
chunks.push({ rel: `preload`, name: asset })
)

const childAssets = namedChunkGroups[chunkKey].childAssets
for (const rel in childAssets) {
chunks = concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the whole point of flatten to not need this? :)

chunks,
childAssets[rel].map(chunk => {
return { rel, name: chunk }
})
)
}

return chunks
})
)
.filter(s => isObject(s))
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 this could be simplified to

Suggested change
.filter(s => isObject(s))
.filter(Boolean)

.sort((s1, s2) => (s1.rel == `preload` ? -1 : 1)) // given priority to preload
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Nit: .sort expects 0 for equal. But I guess that makes the sort function harder. I dunno, maybe ignore this one. The list shouldn't be big to really matter.
  • eslint should have warned about ==, are your post install hooks setup properly?
Suggested change
.sort((s1, s2) => (s1.rel == `preload` ? -1 : 1)) // given priority to preload
.sort((s1, s2) => s1.rel === s2.rel ? 0 : s1.rel === `preload` ? -1 : s2.rel === `preload` ? 1 : 0)) // given priority to preload


scriptsAndStyles = uniqBy(scriptsAndStyles, item => item.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

If scriptsAndStyles was a Map to begin with, it wouldn't need to do that relatively expensive step here. And it can still return an array ([...scriptsAndStyles.values()]).


const styles = scriptsAndStyles.filter(
style => style.name && style.name.endsWith(`.css`)
)
Comment on lines +160 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is future-proofing but scriptsAndStyles is not used after this point. Could it not just be styles?


styles
pieh marked this conversation as resolved.
Show resolved Hide resolved
.slice(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

styles is a fresh array and not used anywhere else. I think this .slice is redundant.

.reverse()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the .sort above not immediately sort this proper?

.forEach(style => {
headComponents.unshift(
<link
data-identity={`gatsby-dev-css`}
pieh marked this conversation as resolved.
Show resolved Hide resolved
key={style.name}
rel="stylesheet"
type="text/css"
href={`${__PATH_PREFIX__}/${style.name}`}
/>
)
})

const createElement = React.createElement

Expand Down
30 changes: 22 additions & 8 deletions packages/gatsby/src/utils/webpack-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,28 @@ export const createWebpackUtils = (
},

miniCssExtract: (options = {}) => {
return {
options,
// use MiniCssExtractPlugin only on production builds
loader: PRODUCTION
? MiniCssExtractPlugin.loader
: require.resolve(`style-loader`),
if (PRODUCTION) {
// production always uses MiniCssExtractPlugin
return {
loader: MiniCssExtractPlugin.loader,
options,
}
} else if (process.env.GATSBY_EXPERIMENTAL_DEV_SSR) {
// develop with ssr also uses MiniCssExtractPlugin
return {
loader: MiniCssExtractPlugin.loader,
options: {
...options,
// enable hmr for browser bundle, ssr bundle doesn't need it
hmr: stage === `develop`,
},
}
} else {
// develop without ssr is using style-loader
return {
loader: require.resolve(`style-loader`),
options,
}
}
},

Expand Down Expand Up @@ -690,8 +706,6 @@ export const createWebpackUtils = (

plugins.extractText = (options: any): Plugin =>
new MiniCssExtractPlugin({
filename: `[name].[contenthash].css`,
chunkFilename: `[name].[contenthash].css`,
...options,
})

Expand Down
13 changes: 12 additions & 1 deletion packages/gatsby/src/utils/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,21 @@ module.exports = async (
plugins.eslintGraphqlSchemaReload(),
])
.filter(Boolean)
if (process.env.GATSBY_EXPERIMENTAL_DEV_SSR) {
// Don't use the default mini-css-extract-plugin setup as that
// breaks hmr.
configPlugins.push(
plugins.extractText({ filename: `[name].css` }),
plugins.extractStats()
)
}
break
case `build-javascript`: {
configPlugins = configPlugins.concat([
plugins.extractText(),
plugins.extractText({
filename: `[name].[contenthash].css`,
chunkFilename: `[name].[contenthash].css`,
}),
// Write out stats object mapping named dynamic imports (aka page
// components) to all their async chunks.
plugins.extractStats(),
Expand Down