-
Notifications
You must be signed in to change notification settings - Fork 10.3k
feature(gatsby): Extract non-css-in-js css and add add to <head> when SSRing in dev #28471
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
Changes from all commits
ee315ed
b94ced1
0228936
135e960
b96c520
fee4f5a
36e7f9e
3357d79
b3bb3b4
64bc7f9
8e5f6f0
468a286
410424c
d27291f
68b32ad
e6b4c39
9ecb314
84a4f2c
3e8fa67
baf7a65
31354f2
92e4656
5a35154
d4ae483
d4d5340
59148ae
08ee647
c1eeff7
1f9fbdc
1125d6c
51bbd3b
4fbb812
e563bda
d59c6e2
627239c
1afed99
3c088a5
139efda
6d3a64b
b1506c0
9c34498
0b78528
a22361f
f01a5fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| module.exports = {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,5 +53,5 @@ describe(`SSR`, () => { | |
| }, 400) | ||
| }, 400) | ||
| }) | ||
| }) | ||
| }, 15000) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| import "./sample.css" |
| 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`, | ||
| }, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| module.exports = { | ||
| plugins: [require("tailwindcss"), require("autoprefixer")], | ||
| } |
| 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; | ||
| } |
| 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> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| @tailwind base; | ||
|
|
||
| @tailwind components; | ||
|
|
||
| @tailwind utilities; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| .hi { | ||
| color: blue; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| import "./test.css" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| module.exports = { | ||
| purge: [ | ||
| './src/**/*.js', | ||
| ], | ||
| theme: { | ||
| extend: {} | ||
| }, | ||
| variants: {}, | ||
| plugins: [] | ||
| } |
| 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" | ||||||
|
|
@@ -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`) | ||||||
|
|
@@ -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`) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you do If the object may not exist, we should use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the whole point of |
||||||
| chunks, | ||||||
| childAssets[rel].map(chunk => { | ||||||
| return { rel, name: chunk } | ||||||
| }) | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| return chunks | ||||||
| }) | ||||||
| ) | ||||||
| .filter(s => isObject(s)) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be simplified to
Suggested change
|
||||||
| .sort((s1, s2) => (s1.rel == `preload` ? -1 : 1)) // given priority to preload | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| scriptsAndStyles = uniqBy(scriptsAndStyles, item => item.name) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||||||
|
|
||||||
| const styles = scriptsAndStyles.filter( | ||||||
| style => style.name && style.name.endsWith(`.css`) | ||||||
| ) | ||||||
|
Comment on lines
+160
to
+162
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is future-proofing but |
||||||
|
|
||||||
| styles | ||||||
pieh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| .slice(0) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| .reverse() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the |
||||||
| .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 | ||||||
|
|
||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.