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.

Sign up for GitHub

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

Jump to bottom

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

feature(gatsby): Extract non-css-in-js css and add add to <head> when SSRing in dev #28471

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
get tests passing
  • Loading branch information
@KyleAMathews
KyleAMathews committed Dec 4, 2020
commit b3bb3b492c39ba51c298d19af19a07cd853ee6bf
9 changes: 8 additions & 1 deletion integration-tests/ssr/__tests__/__snapshots__/ssr.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
// 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\\"/><style data-href=\\"/main.398bf18ace04c4c482d6.css\\" id=\\"gatsby-global-css\\">body {
background: pink;
}


/*# sourceMappingURL=main.398bf18ace04c4c482d6.css.map*/</style><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 it generates an error page correctly 1`] = `
"<head>
Expand Down
1 change: 1 addition & 0 deletions integration-tests/ssr/src/pages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from "react"
import { useStaticQuery, graphql } from "gatsby"
import "./sample.css"

export default function Inline() {
const { site } = useStaticQuery(graphql`
Expand Down
3 changes: 3 additions & 0 deletions integration-tests/ssr/src/pages/sample.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
background: pink;
}
22 changes: 20 additions & 2 deletions packages/gatsby/cache-dir/__tests__/static-entry.js
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions packages/gatsby/cache-dir/ssr-develop-static-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ export default (pagePath, isClientOnlyPage, callback) => {
const { componentChunkName, staticQueryHashes = [] } = pageData

let scriptsAndStyles = flatten(
[`app`, componentChunkName].map(s => {
const fetchKey = `assetsByChunkName[${s}]`
[`main`].map(chunkKey => {
const fetchKey = `assetsByChunkName[${chunkKey}]`

let chunks = get(stats, fetchKey, [])
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 •
edited
Loading

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) {
Expand All @@ -135,7 +135,7 @@ export default (pagePath, isClientOnlyPage, callback) => {
return { rel: `preload`, name: chunk }
})

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

Expand Down