refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle #29396

pieh · 2021-02-08T16:51:37Z

Description

Documentation

Related Issues

[ch23634]

pieh · 2021-02-08T16:52:09Z

packages/babel-plugin-remove-graphql-queries/src/index.ts

-                state.opts.stage === `develop-html` ||
-                state.opts.stage === `build-html`) &&
+                state.opts.stage === `develop-html`) &&


breaking change no. 1 - babel plugin no longer inline static queries in build-html

Just curious, why only build-html? It is not immediately clear.

For the most part because I don't "care" about develop-html in this PR (other than it still working heh).

I remove static queries results inlining from Prod SSR bundle so it doesn't impact SSR bundle compilation hash so we don't hit case of SSR bundle hash changing invalidating ALL the pages (so we can invalidate only pages that actually are affected by static query result changes).

pieh · 2021-02-08T16:53:24Z

packages/gatsby/cache-dir/static-entry.js

-    return null
-  }
+function loadPageDataSync() {
+  throw new Error(`"loadPageDataSync" is no longer available`)


Breaking change no. 2

This might not need to be done, but only plugin that was making use of this was guess-js one

This might be changed to "temporarairly unavailble, but using this will cause all pages to be rebuild all the time" when we wrap fs usage.

That would be safer given, that we didn't have deprecation for it, so we don't need to break it completely (so it continue to work but usage of it would disallow using partial html writes)

https://www.npmjs.com/package/gatsby-plugin-guess-js gets almost no downloads so I think it's fine saying adios.

pieh · 2021-02-09T09:01:53Z

packages/gatsby/src/utils/start-server.ts

-        res.sendFile(directoryPath(`public/index.html`), err => {
+        res.sendFile(directoryPath(`.cache/develop-html/index.html`), err => {


Just so develop skeleton index.html doesn't override prod one in public dir

(and also this would be removed when DEV_SSR is stable)

pieh · 2021-02-09T09:03:48Z

packages/gatsby/src/services/initialize.ts

@@ -586,6 +593,7 @@ export async function initialize({
    .map(
      plugin =>
        `{
+      name: '${plugin.name}',


Added only to decorate thrown error with plugin name when there is exception during gatsby-ssr execution (like trying to use removed loadPageDataSync helper

packages/gatsby/cache-dir/static-entry.js

packages/gatsby/src/utils/worker/render-html.ts

vladar · 2021-02-10T09:44:04Z

packages/babel-plugin-remove-graphql-queries/src/index.ts

-                state.opts.stage === `develop-html` ||
-                state.opts.stage === `build-html`) &&
+                state.opts.stage === `develop-html`) &&


Just curious, why only build-html? It is not immediately clear.

packages/gatsby/cache-dir/static-entry.js

vladar · 2021-02-10T10:27:59Z

packages/gatsby/src/utils/worker/render-html.ts

+  let scriptsAndStyles = flatten(
+    [`app`, componentChunkName].map(s => {


Nit: I think we can use ["app", "componentChunkName"].flatMap() now that the minimum Node version is 12.13

vladar · 2021-02-10T10:38:53Z

packages/gatsby/src/utils/worker/render-html.ts

+      chunks = chunks.map(chunk => {
+        if (chunk === `/`) {
+          return null
+        }
+        return { rel: `preload`, name: chunk }
+      })
+
+      namedChunkGroups[s].assets.forEach(asset =>
+        chunks.push({ rel: `preload`, name: asset })
+      )
+
+      const childAssets = namedChunkGroups[s].childAssets
+      for (const rel in childAssets) {
+        chunks = concat(
+          chunks,
+          childAssets[rel].map(chunk => {
+            return { rel, name: chunk }
+          })
+        )
+      }


Nit: to build this final chunks array we first use map, then forEach with push and then for with concat 😄 Maybe prettify for consistency?

I realize this was originally this way in static-entry.js but I doubt we'll find a better time to refactor it a bit :)

yeah, this was straight c&p, but we can use this opportunity to streamline this code (if not now then we never will heh)

TBH, I'll probably not use any "functional" array methods and just init array(s) ahead of time and do for loops adding as many things as needed inside.

done in 1e87c41 + follow up fix for 1 thing I messed up in 7c46317

vladar · 2021-02-10T10:53:42Z

packages/gatsby/src/utils/worker/render-html.ts

+          if (style.rel !== `prefetch`) {
+            const memoizedInlineCss = inlineCssCache.get(style.name)
+            if (memoizedInlineCss) {
+              return {
+                ...style,
+                content: memoizedInlineCss,
+              }
+            }
+
+            let getInlineCssPromise = inFlightInlineCssPromise.get(style.name)
+            if (!getInlineCssPromise) {
+              getInlineCssPromise = fs
+                .readFile(join(process.cwd(), `public`, style.name), `utf-8`)
+                .then(content => {
+                  inlineCssCache.set(style.name, content)
+                  inFlightInlineCssPromise.delete(style.name)
+
+                  return content
+                })
+
+              inFlightInlineCssPromise.set(style.name, getInlineCssPromise)
+            }
+
+            return {
+              ...style,
+              content: await getInlineCssPromise,
+            }


Nit: do we need this inlineCssCache cache? Maybe we can just keep inlineCssPromiseCache to simplify this a bit?

Suggested change

if (style.rel !== `prefetch`) {

const memoizedInlineCss = inlineCssCache.get(style.name)

if (memoizedInlineCss) {

return {

...style,

content: memoizedInlineCss,

}

}

let getInlineCssPromise = inFlightInlineCssPromise.get(style.name)

if (!getInlineCssPromise) {

getInlineCssPromise = fs

.readFile(join(process.cwd(), `public`, style.name), `utf-8`)

.then(content => {

inlineCssCache.set(style.name, content)

inFlightInlineCssPromise.delete(style.name)

return content

})

inFlightInlineCssPromise.set(style.name, getInlineCssPromise)

}

return {

...style,

content: await getInlineCssPromise,

}

if (style.rel !== `prefetch`) {

let getInlineCssPromise = inFlightInlineCssPromise.get(style.name)

if (!getInlineCssPromise) {

getInlineCssPromise = fs

.readFile(join(process.cwd(), `public`, style.name), `utf-8`)

inFlightInlineCssPromise.set(style.name, getInlineCssPromise)

}

return {

...style,

content: await getInlineCssPromise,

}

We still preserve inlineCssCache in memory so not a big difference. Or am I missing something?

No, you are right. This was just poor attempt at avoiding promise usage when not needed, but given that this would be used at most once per template there's no measurable diff there to gain and simplifying things is for the best

done in f9016bb

…from ssr bundle

… used for

…ating urls

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>

…e issues with creating pretty error message with codeframes etc

vladar

Let's do it! 🚢

…from ssr bundle (gatsbyjs#29396) * refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle * print plugin name that thrown * rename timestamp to sessionId to better convey semantics on what it's used for * use posix joins, so builds on windows dont use backslashes when generating urls * no moar lodash * simplify inline css caches by merging 2 caches into 1 promise cache Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com> * apparently css is not scripting language * set default param value * add plugin annotation to message, don't create new error as this cause issues with creating pretty error message with codeframes etc Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>

pieh added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Feb 8, 2021

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 8, 2021

pieh commented Feb 8, 2021

View reviewed changes

pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 8, 2021

pieh force-pushed the no-fs-in-ssr-no-static-queries-in-ssr-bundle branch 2 times, most recently from 98d992e to 2fb0aa1 Compare February 8, 2021 19:38

pieh marked this pull request as ready for review February 9, 2021 08:58

pieh commented Feb 9, 2021

View reviewed changes

pieh added the topic: SSG* label Feb 9, 2021

vladar reviewed Feb 10, 2021

View reviewed changes

packages/gatsby/cache-dir/static-entry.js Show resolved Hide resolved

vladar reviewed Feb 10, 2021

View reviewed changes

pieh force-pushed the no-fs-in-ssr-no-static-queries-in-ssr-bundle branch from 33a2caa to 9a74e9e Compare February 11, 2021 23:30

This was referenced Feb 12, 2021

refactor(conditional-page-build): tracking PR #29140

Closed

refactor(conditional-page-build): explicitly persist status of generated html files #29473

Merged

pieh added the topic: build Related to the Gatsby build process label Feb 14, 2021

pieh and others added 9 commits February 15, 2021 12:45

refactor(html-ssr): remove fs from ssr bundle, remove static queries …

f8ab52d

…from ssr bundle

print plugin name that thrown

2ff695b

rename timestamp to sessionId to better convey semantics on what it's…

04ca3d9

… used for

use posix joins, so builds on windows dont use backslashes when gener…

7c8f0fe

…ating urls

no moar lodash

f023415

simplify inline css caches by merging 2 caches into 1 promise cache

0ade9c3

Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>

apparently css is not scripting language

6d5c843

set default param value

57e7eac

add plugin annotation to message, don't create new error as this caus…

aac9d30

…e issues with creating pretty error message with codeframes etc

pieh force-pushed the no-fs-in-ssr-no-static-queries-in-ssr-bundle branch from 0c83fdc to aac9d30 Compare February 15, 2021 11:50

vladar approved these changes Feb 15, 2021

View reviewed changes

pieh merged commit f221c33 into master Feb 15, 2021

pieh deleted the no-fs-in-ssr-no-static-queries-in-ssr-bundle branch February 15, 2021 17:28

pieh mentioned this pull request Feb 24, 2021

fix: re-implement loadPageDataSync for onRenderBody in gatsby-ssr #29734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle #29396

refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle #29396

pieh commented Feb 8, 2021 •

edited

Loading

pieh Feb 8, 2021 •

edited

Loading

vladar Feb 10, 2021

pieh Feb 10, 2021

pieh Feb 8, 2021 •

edited

Loading

pieh Feb 9, 2021

KyleAMathews Feb 9, 2021

pieh Feb 9, 2021

pieh Feb 9, 2021

pieh Feb 9, 2021

vladar Feb 10, 2021

vladar Feb 10, 2021

vladar Feb 10, 2021

pieh Feb 10, 2021

pieh Feb 11, 2021

pieh Feb 11, 2021

vladar Feb 10, 2021

pieh Feb 10, 2021

pieh Feb 11, 2021

vladar left a comment

		res.sendFile(directoryPath(`public/index.html`), err => {
		res.sendFile(directoryPath(`.cache/develop-html/index.html`), err => {

		let scriptsAndStyles = flatten(
		[`app`, componentChunkName].map(s => {

refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle #29396

refactor(html-ssr): remove fs from ssr bundle, remove static queries from ssr bundle #29396

Conversation

pieh commented Feb 8, 2021 • edited Loading

Description

Documentation

Related Issues

pieh Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieh Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladar left a comment

Choose a reason for hiding this comment

pieh commented Feb 8, 2021 •

edited

Loading

pieh Feb 8, 2021 •

edited

Loading

pieh Feb 8, 2021 •

edited

Loading