Add Gzip support for JSON pages #571

arunoda · 2016-12-30T10:02:36Z

Related to #555
Fixes #568

Now we are serving JSON pages directly from the file system for most of the time. We do that with a custom webpack plugin.

So, now we could add gzip support for pages pretty quickly.
Since now we serve JSON pages directly from the filesystem, ETags are sent automatically and it fixes #568

arunoda · 2016-12-30T10:03:45Z

server/build/plugins/json-pages-plugin.js

@@ -0,0 +1,22 @@
+export default class JsonPagesPlugin {


@nkzawa This is my first webpack plugin.
Could you check whether I have done something ugly here?

looks good to me 👍

arunoda · 2016-12-30T10:04:51Z

server/build/gzip.js

+
+  while (true) {
+    // gzip only 10 assets in parallel at a time.
+    const currentChunk = allAssets.splice(0, 10)


@rauchg it didn't sounds nice to gzip all at once in parallel to me.
Is this something I don't wanna worry?

nkzawa · 2016-12-30T11:10:01Z

server/build/gzip.js

+      resolve(pages)
+    })
+  })
+}


Why we don't use glob-promise ?

Yeah! I'll try to use it.

nkzawa · 2016-12-30T11:11:39Z

server/build/gzip.js

+    const currentChunk = allAssets.splice(0, 10)
+    if (currentChunk.length === 0) break
+
+    const promises = currentChunk.map((filePath) => gzip(filePath))


It can be currentChunk.map(filePath) ?

Didn't get this?

I meant currentChunk.map(gzip)

nkzawa · 2016-12-30T11:20:51Z

server/build/plugins/json-pages-plugin.js

+        const newContent = JSON.stringify({ component: content })
+        page.source = () => newContent
+        page.size = () => newContent.length
+        page.jsoned = true


Should we change the extension to .json ?

Yes. That can be done. I'll try it.

nkzawa · 2016-12-30T11:22:20Z

server/render.js

+    send(req, path)
+    .on('error', (err) => {
+      if (err.code === 'ENOENT') {
+        this.render404(req, res).then(resolve, reject)


We can't resolve this.

Good catch. I'll move that to here too.

nkzawa · 2016-12-30T11:23:03Z

server/render.js

  ] = await Promise.all([
    Component.getInitialProps ? Component.getInitialProps(ctx) : {},
    read(join(dir, '.next', 'bundles', 'pages', page)),
    read(join(dir, '.next', 'bundles', 'pages', dev ? '_error-debug' : '_error'))
  ])

+  const component = JSON.parse(componentJson).component
+  const errorComponent = JSON.parse(errorComponentJson).component


We should cache the parsed result.

arunoda · 2016-12-30T15:26:52Z

@nkzawa Implemented all the suggestion you made.

arunoda · 2016-12-30T15:28:47Z

client/webpack-hot-middleware-client.js

@@ -41,7 +41,8 @@ webpackHotMiddlewareClient.subscribe((obj) => {
  const fn = handlers[obj.action]
  if (fn) {
    const data = obj.data || []
-    fn(...data)
+    const route = data[0].replace(/[index]*\.json$/, '')


@nkzawa
I had to do this because now these files routes have .json extension.
Do you know why this has changed?
Is there any better way to deal with this?

Please change server/hot-reloader.js instead of this file. Maybe toRoute method.

nkzawa · 2016-12-31T02:22:02Z

server/build/webpack.js

@@ -32,7 +34,7 @@ export default async function createCompiler (dir, { dev = false, quiet = false
  const defaultEntries = dev
    ? [join(__dirname, '..', '..', 'client/webpack-hot-middleware-client')] : []
  for (const p of pages) {
-    entry[join('bundles', p)] = defaultEntries.concat([`./${p}?entry`])
+    entry[pagePathToJson(join('bundles', p))] = defaultEntries.concat([`./${p}?entry`])


I think, ideally, extensions should be changed on JsonPagesPlugin. Is it difficult ?

Nope. That's not hard. I think that makes sense. I'll do it.

nkzawa · 2016-12-31T02:24:39Z

server/read.js

-export default read
-export const cache = {}
-
-read.cache = cache


This is used on hot-reloader.js too. I think we need to fix it.

nkzawa · 2016-12-31T02:26:42Z

server/render.js

+    .on('error', (err) => {
+      if (err.code === 'ENOENT') {
+        res.statusCode = 404
+        res.end('Not Found')


Maybe this change is acceptable, but it'd be better if we can show the original 404.

In the orginal 404, it sends a bunch of HTML too. It's okay for pages, but this is just a JSON endpoint. We don't need that. (Since user won't access these files directly)

This is not only for JSON endpoint, but it's also for /static. Users might access old static file URL and see 404. Anyway I think it's ok for now.

I'll work a separate PR for that.

nkzawa · 2016-12-31T05:57:27Z

server/build/plugins/json-pages-plugin.js

+
+      // Delete existing json pages
+      // Otherwise, we might keep JSON pages for deleted pages.
+      jsonPages.forEach((pagePath) => delete compilation.assets[pagePath])


I think we don't need this since compilation instance is recreated on each build. So I think there is no json pages remaining, or is there ?

Yep. Removed.

nkzawa · 2016-12-31T06:09:16Z

server/render.js

+  }
+
+  const gzipPath = `${path}.gz`
+  const exists = await fs.exists(gzipPath)


this is a deprecated API.

Oh yeah! Seems like I need to read along the Node.js API again :)

Seems like fs.access is not recommended for this: https://nodejs.org/api/fs.html#fs_fs_access_path_mode_callback

I'll work on with renderStatic for this to get rid of this issue.

@nkzawa Check now.
I figured, it's safe for us to use fs.access

nkzawa · 2016-12-31T12:46:21Z

👍 amazing

msand · 2017-01-01T16:53:48Z

server/build/plugins/json-pages-plugin.js

+    compiler.plugin('after-compile', (compilation, callback) => {
+      const pages = Object
+        .keys(compilation.assets)
+        .filter((filename) => /^bundles\/pages.*\.js$/.test(filename))


This regex doesn't match the way one could wish for in windows, causing pages not to be compiled to json, causing issue #597 thus making most tests fail on windows.

Thanks @msand for figuring this out.

arunoda added 6 commits December 30, 2016 12:12

Server JSON pages directly from the filesystem.

bc2774a

Make Json pages even if there's an error.

b6daa28

Implement much better page serving.

6dd5db3

Use JsonPagesPlugin in the production mode as well.

bff2435

Add gzip support for JSON pages.

901915d

Merge master.

6401e33

arunoda commented Dec 30, 2016

View reviewed changes

rauchg mentioned this pull request Dec 30, 2016

Ahead-of-time GZIP compression #555

Closed

3 tasks

nkzawa reviewed Dec 30, 2016

View reviewed changes

arunoda added 6 commits December 30, 2016 17:33

Use glob-promise instead of recursive-readdir

3af72f1

Handle renderStatic 404 properly.

81ba173

Simply the gzip code.

298629f

Cache already read JSON pages.

b0b04a7

Change JSON pages extension to .json.

1e0e7a8

Fix HMR related issue.

e24f261

arunoda commented Dec 30, 2016

View reviewed changes

rauchg assigned nkzawa Dec 30, 2016

nkzawa reviewed Dec 31, 2016

View reviewed changes

arunoda added 3 commits December 31, 2016 10:35

Fix hot-reload for .json solely on server.

7a04f38

Properly clear cache on hot-reloader.

79d566c

Convert .js pages into .json page right inside the plugin.

352fe77

nkzawa reviewed Dec 31, 2016

View reviewed changes

arunoda added 3 commits December 31, 2016 11:44

Fix gzipping .json pages.

20d01ba

Remove unwanted json pages cleanup.

343cb92

Get rid of deprecated fs.exists for fs.access

370189b

nkzawa merged commit 165924b into vercel:master Dec 31, 2016

arunoda deleted the gzip-on-json-pages branch December 31, 2016 13:04

msand reviewed Jan 1, 2017

View reviewed changes

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Add Gzip support for JSON pages #571

Add Gzip support for JSON pages #571

Conversation

arunoda commented Dec 30, 2016

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

arunoda commented Dec 30, 2016

arunoda Dec 30, 2016 • 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

nkzawa Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkzawa Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkzawa Dec 31, 2016 • 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

nkzawa commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda Dec 30, 2016 •

edited

Loading

nkzawa Dec 31, 2016 •

edited

Loading

nkzawa Dec 31, 2016 •

edited

Loading

nkzawa Dec 31, 2016 •

edited

Loading