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

Add Gzip support for JSON pages #571

Merged
merged 18 commits into from
Dec 31, 2016
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
3 changes: 2 additions & 1 deletion client/webpack-hot-middleware-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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$/, '')
Copy link
Contributor Author

@arunoda arunoda Dec 30, 2016

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

fn(route)
} else {
throw new Error('Unexpected action ' + obj.action)
}
Expand Down
4 changes: 4 additions & 0 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,7 @@ export function deprecated (fn, message) {

return newFn
}

export function pagePathToJson (pagePath) {
return pagePath.replace(/\.js$/, '.json')
}
23 changes: 19 additions & 4 deletions server/build/gzip.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import fs from 'fs'
import path from 'path'
import zlib from 'zlib'
import glob from 'glob-promise'

export default async function gzipAssets (dir) {
const nextDir = path.resolve(dir, '.next')

await Promise.all([
gzip(path.resolve(nextDir, 'commons.js')),
gzip(path.resolve(nextDir, 'main.js'))
])
const coreAssets = [
path.join(nextDir, 'commons.js'),
path.join(nextDir, 'main.js')
]
const pages = await glob('bundles/pages/**/*.js', { cwd: nextDir })

const allAssets = [
...coreAssets,
...pages.map(page => path.join(nextDir, page))
]

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

Choose a reason for hiding this comment

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

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

if (currentChunk.length === 0) break

await Promise.all(currentChunk.map(gzip))
}
}

export function gzip (filePath) {
Expand Down
22 changes: 22 additions & 0 deletions server/build/plugins/json-pages-plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
export default class JsonPagesPlugin {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good to me 👍

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

pages.forEach((pageName) => {
const page = compilation.assets[pageName]
if (page.jsoned) return

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

Choose a reason for hiding this comment

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

Should we change the extension to .json ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

})

callback()
})
}
}
9 changes: 6 additions & 3 deletions server/build/webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import WatchPagesPlugin from './plugins/watch-pages-plugin'
import WatchRemoveEventPlugin from './plugins/watch-remove-event-plugin'
import DynamicEntryPlugin from './plugins/dynamic-entry-plugin'
import DetachPlugin from './plugins/detach-plugin'
import JsonPagesPlugin from './plugins/json-pages-plugin'
import getConfig from '../config'
import { pagePathToJson } from '../../lib/utils'

const documentPage = join('pages', '_document.js')
const defaultPages = [
Expand All @@ -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`])
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, ideally, extensions should be changed on JsonPagesPlugin. Is it difficult ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

const nextPagesDir = join(__dirname, '..', '..', 'pages')
Expand All @@ -42,7 +44,7 @@ export default async function createCompiler (dir, { dev = false, quiet = false
const entryName = join('bundles', 'pages', p)
const path = join(nextPagesDir, p)
if (!entry[entryName]) {
entry[entryName] = defaultEntries.concat([path + '?entry'])
entry[pagePathToJson(entryName)] = defaultEntries.concat([path + '?entry'])
}
interpolateNames.set(path, `dist/pages/${p}`)
}
Expand All @@ -69,7 +71,8 @@ export default async function createCompiler (dir, { dev = false, quiet = false
name: 'commons',
filename: 'commons.js',
minChunks: Math.max(2, minChunks)
})
}),
new JsonPagesPlugin()
]

if (dev) {
Expand Down
68 changes: 18 additions & 50 deletions server/index.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import { resolve, join } from 'path'
import { parse } from 'url'
import http from 'http'
import fs from 'mz/fs'
import send from 'send'
import accepts from 'accepts'
import {
renderToHTML,
renderErrorToHTML,
renderJSON,
renderErrorJSON,
sendHTML
sendHTML,
serveStatic,
serveStaticWithGzip
} from './render'
import Router from './router'
import HotReloader from './hot-reloader'
Expand Down Expand Up @@ -58,32 +57,32 @@ export default class Server {
defineRoutes () {
this.router.get('/_next-prefetcher.js', async (req, res, params) => {
const p = join(__dirname, '../client/next-prefetcher-bundle.js')
await this.serveStatic(req, res, p)
await serveStatic(req, res, p)
})

this.router.get('/_next/main.js', async (req, res, params) => {
const p = join(this.dir, '.next/main.js')
await this.serveStaticWithGzip(req, res, p)
await serveStaticWithGzip(req, res, p)
})

this.router.get('/_next/commons.js', async (req, res, params) => {
const p = join(this.dir, '.next/commons.js')
await this.serveStaticWithGzip(req, res, p)
await serveStaticWithGzip(req, res, p)
})

this.router.get('/_next/pages/:path*', async (req, res, params) => {
const paths = params.path || []
const paths = params.path || ['index']
const pathname = `/${paths.join('/')}`
await this.renderJSON(res, pathname)
await this.renderJSON(req, res, pathname)
})

this.router.get('/_next/:path+', async (req, res, params) => {
const p = join(__dirname, '..', 'client', ...(params.path || []))
await this.serveStatic(req, res, p)
await serveStatic(req, res, p)
})
this.router.get('/static/:path+', async (req, res, params) => {
const p = join(this.dir, 'static', ...(params.path || []))
await this.serveStatic(req, res, p)
await serveStatic(req, res, p)
})

this.router.get('/:path*', async (req, res) => {
Expand Down Expand Up @@ -180,69 +179,38 @@ export default class Server {
this.renderErrorToHTML(null, req, res, pathname, query)
}

async renderJSON (res, page) {
async renderJSON (req, res, page) {
if (this.dev) {
const compilationErr = this.getCompilationError(page)
if (compilationErr) {
return this.renderErrorJSON(compilationErr, res)
return this.renderErrorJSON(compilationErr, req, res)
}
}

try {
await renderJSON(res, page, this.renderOpts)
await renderJSON(req, res, page, this.renderOpts)
} catch (err) {
if (err.code === 'ENOENT') {
res.statusCode = 404
return this.renderErrorJSON(null, res)
return this.renderErrorJSON(null, req, res)
} else {
if (!this.quiet) console.error(err)
res.statusCode = 500
return this.renderErrorJSON(err, res)
return this.renderErrorJSON(err, req, res)
}
}
}

async renderErrorJSON (err, res) {
async renderErrorJSON (err, req, res) {
if (this.dev) {
const compilationErr = this.getCompilationError('/_error')
if (compilationErr) {
res.statusCode = 500
return renderErrorJSON(compilationErr, res, this.renderOpts)
return renderErrorJSON(compilationErr, req, res, this.renderOpts)
}
}

return renderErrorJSON(err, res, this.renderOpts)
}

async serveStaticWithGzip (req, res, path) {
const encoding = accepts(req).encodings(['gzip'])
if (encoding !== 'gzip') {
return this.serveStatic(req, res, path)
}

const gzipPath = `${path}.gz`
const exists = await fs.exists(gzipPath)
if (!exists) {
return this.serveStatic(req, res, path)
}

res.setHeader('Content-Encoding', 'gzip')
return this.serveStatic(req, res, gzipPath)
}

serveStatic (req, res, path) {
return new Promise((resolve, reject) => {
send(req, path)
.on('error', (err) => {
if (err.code === 'ENOENT') {
this.render404(req, res).then(resolve, reject)
} else {
reject(err)
}
})
.pipe(res)
.on('finish', resolve)
})
return renderErrorJSON(err, req, res, this.renderOpts)
}

getCompilationError (page) {
Expand Down
25 changes: 25 additions & 0 deletions server/read-page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import fs from 'mz/fs'
import resolve from './resolve'

/**
* resolve a JSON page like `require.resolve`,
* and read and cache the file content
*/

async function readPage (path) {
const f = await resolve(path)
if (cache.hasOwnProperty(f)) {
return cache[f]
}

const source = await fs.readFile(f, 'utf8')
const { component } = JSON.parse(source)

cache[f] = component
return component
}

export default readPage
export const cache = {}

readPage.cache = cache
23 changes: 0 additions & 23 deletions server/read.js

This file was deleted.

53 changes: 45 additions & 8 deletions server/render.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { join } from 'path'
import { createElement } from 'react'
import { renderToString, renderToStaticMarkup } from 'react-dom/server'
import fs from 'mz/fs'
import send from 'send'
import accepts from 'accepts'
import requireModule from './require'
import read from './read'
import readPage from './read-page'
import { Router } from '../lib/router'
import Head, { defaultHead } from '../lib/head'
import App from '../lib/app'
Expand Down Expand Up @@ -48,8 +51,8 @@ async function doRender (req, res, pathname, query, {
errorComponent
] = 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'))
readPage(join(dir, '.next', 'bundles', 'pages', page)),
readPage(join(dir, '.next', 'bundles', 'pages', dev ? '_error-debug' : '_error'))
])

// the response might be finshed on the getinitialprops call
Expand Down Expand Up @@ -93,14 +96,15 @@ async function doRender (req, res, pathname, query, {
return '<!DOCTYPE html>' + renderToStaticMarkup(doc)
}

export async function renderJSON (res, page, { dir = process.cwd() } = {}) {
const component = await read(join(dir, '.next', 'bundles', 'pages', page))
sendJSON(res, { component })
export async function renderJSON (req, res, page, { dir = process.cwd() } = {}) {
const pagePath = join(dir, '.next', 'bundles', 'pages', `${page}.json`)
return serveStaticWithGzip(req, res, pagePath)
}

export async function renderErrorJSON (err, res, { dir = process.cwd(), dev = false } = {}) {
export async function renderErrorJSON (err, req, res, { dir = process.cwd(), dev = false } = {}) {
const page = err && dev ? '/_error-debug' : '/_error'
const component = await read(join(dir, '.next', 'bundles', 'pages', page))
const component = await readPage(join(dir, '.next', 'bundles', 'pages', page))

sendJSON(res, {
component,
err: err && dev ? errorToJSON(err) : null
Expand Down Expand Up @@ -132,3 +136,36 @@ function errorToJSON (err) {

return json
}

export async function serveStaticWithGzip (req, res, path) {
const encoding = accepts(req).encodings(['gzip'])
if (encoding !== 'gzip') {
return serveStatic(req, res, path)
}

const gzipPath = `${path}.gz`
const exists = await fs.exists(gzipPath)
Copy link
Contributor

@nkzawa nkzawa Dec 31, 2016

Choose a reason for hiding this comment

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

this is a deprecated API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (!exists) {
return serveStatic(req, res, path)
}

res.setHeader('Content-Encoding', 'gzip')
return serveStatic(req, res, gzipPath)
}

export function serveStatic (req, res, path) {
return new Promise((resolve, reject) => {
send(req, path)
.on('error', (err) => {
if (err.code === 'ENOENT') {
res.statusCode = 404
res.end('Not Found')
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 change is acceptable, but it'd be better if we can show the original 404.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@nkzawa nkzawa Dec 31, 2016

Choose a reason for hiding this comment

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

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.

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'll work a separate PR for that.

resolve()
} else {
reject(err)
}
})
.pipe(res)
.on('finish', resolve)
})
}
Loading