-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Implement "on demand entries" #1111
Changes from 32 commits
b295c24
3d975ef
93b0967
e237580
04754a8
b2383e1
93fc9d6
dd5fb13
39219d0
fc0a54f
617c33f
cf0da3f
f40d425
0ab1c28
620e838
bea21d5
d32c56a
8a495aa
30b6351
fbca518
44d65b2
e5cf79b
acaa301
328cf2e
0256e9d
349579a
ddf6a0c
d11c5b3
f706a49
a6df1cc
3166858
291c827
07605a1
4278a18
f87f796
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,36 @@ | ||
/* global location */ | ||
|
||
import Router from '../lib/router' | ||
import fetch from 'unfetch' | ||
|
||
// Ping on every page change | ||
const originalOnRouteChangeComplete = Router.onRouteChangeComplete | ||
Router.onRouteChangeComplete = function (...args) { | ||
ping() | ||
if (originalOnRouteChangeComplete) originalOnRouteChangeComplete(...args) | ||
} | ||
|
||
async function ping () { | ||
try { | ||
const url = `/on-demand-entries-ping?page=${Router.pathname}` | ||
const res = await fetch(url) | ||
const payload = await res.json() | ||
if (payload.invalid) { | ||
location.reload() | ||
} | ||
} catch (err) { | ||
console.error(`Error with on-demand-entries-ping: ${err.message}`) | ||
} | ||
} | ||
|
||
async function runPinger () { | ||
while (true) { | ||
await new Promise((resolve) => setTimeout(resolve, 5000)) | ||
await ping() | ||
} | ||
} | ||
|
||
runPinger() | ||
.catch((err) => { | ||
console.error(err) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,184 @@ | ||
import DynamicEntryPlugin from 'webpack/lib/DynamicEntryPlugin' | ||
import { EventEmitter } from 'events' | ||
import { join } from 'path' | ||
import { parse } from 'url' | ||
import resolvePath from './resolve' | ||
import touch from 'touch' | ||
|
||
const ADDED = Symbol() | ||
const BUILDING = Symbol() | ||
const BUILT = Symbol() | ||
|
||
export default function onDemandEntryHandler (devMiddleware, compiler, { | ||
dir, | ||
dev, | ||
maxInactiveAge = 1000 * 25 | ||
}) { | ||
const entries = {} | ||
const lastAccessPages = [''] | ||
const doneCallbacks = new EventEmitter() | ||
let touchedAPage = false | ||
|
||
compiler.plugin('make', function (compilation, done) { | ||
const allEntries = Object.keys(entries).map((page) => { | ||
const { name, entry } = entries[page] | ||
entries[page].status = BUILDING | ||
return addEntry(compilation, this.context, name, entry) | ||
}) | ||
|
||
Promise.all(allEntries) | ||
.then(() => done()) | ||
.catch(done) | ||
}) | ||
|
||
compiler.plugin('done', function (stats) { | ||
// Call all the doneCallbacks | ||
Object.keys(entries).forEach((page) => { | ||
const entryInfo = entries[page] | ||
if (entryInfo.status !== BUILDING) return | ||
|
||
// With this, we are triggering a filesystem based watch trigger | ||
// It'll memorize some timestamp related info related to common files used | ||
// in the page | ||
// That'll reduce the page building time significantly. | ||
if (!touchedAPage) { | ||
setTimeout(() => { | ||
touch.sync(entryInfo.pathname) | ||
}, 0) | ||
touchedAPage = true | ||
} | ||
|
||
entryInfo.status = BUILT | ||
entries[page].lastActiveTime = Date.now() | ||
doneCallbacks.emit(page) | ||
}) | ||
}) | ||
|
||
setInterval(function () { | ||
disposeInactiveEntries(devMiddleware, entries, lastAccessPages, maxInactiveAge) | ||
}, 5000) | ||
|
||
return { | ||
async ensurePage (page) { | ||
page = normalizePage(page) | ||
|
||
const pagePath = join(dir, 'pages', page) | ||
const pathname = await resolvePath(pagePath) | ||
const name = join('bundles', pathname.substring(dir.length)) | ||
|
||
const entry = [ | ||
join(__dirname, '..', 'client/webpack-hot-middleware-client'), | ||
join(__dirname, '..', 'client', 'on-demand-entries-client'), | ||
`${pathname}?entry` | ||
] | ||
|
||
await new Promise((resolve, reject) => { | ||
const entryInfo = entries[page] | ||
|
||
if (entryInfo) { | ||
if (entryInfo.status === BUILT) { | ||
resolve() | ||
return | ||
} | ||
|
||
if (entryInfo.status === BUILDING) { | ||
doneCallbacks.on(page, processCallback) | ||
return | ||
} | ||
} | ||
|
||
console.log(`> Building page: ${page}`) | ||
|
||
entries[page] = { name, entry, pathname, status: ADDED } | ||
doneCallbacks.on(page, processCallback) | ||
|
||
devMiddleware.invalidate() | ||
|
||
function processCallback (err) { | ||
if (err) return reject(err) | ||
resolve() | ||
} | ||
}) | ||
}, | ||
|
||
middleware () { | ||
return function (req, res, next) { | ||
if (!/^\/on-demand-entries-ping/.test(req.url)) return next() | ||
|
||
const { query } = parse(req.url, true) | ||
const page = normalizePage(query.page) | ||
const entryInfo = entries[page] | ||
|
||
// If there's no entry. | ||
// Then it seems like an weird issue. | ||
if (!entryInfo) { | ||
const message = `Client pings, but there's no entry for page: ${page}` | ||
console.error(message) | ||
sendJson(res, { invalid: true }) | ||
return | ||
} | ||
|
||
// We don't need to maintain active state of anything other than BUILT entries | ||
if (entryInfo.status !== BUILT) return | ||
|
||
// If there's an entryInfo | ||
lastAccessPages.pop() | ||
lastAccessPages.unshift(page) | ||
entryInfo.lastActiveTime = Date.now() | ||
|
||
sendJson(res, { success: true }) | ||
} | ||
} | ||
} | ||
} | ||
|
||
function addEntry (compilation, context, name, entry) { | ||
return new Promise((resolve, reject) => { | ||
const dep = DynamicEntryPlugin.createDependency(entry, name) | ||
compilation.addEntry(context, dep, name, (err) => { | ||
if (err) return reject(err) | ||
resolve() | ||
}) | ||
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 we should not use private plugins. 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. This is not a private function. It's documented by webpack. Anyway, I'll try to use dynamic functions in the refactoring stage. 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 meant about plugins 'webpack/lib/DynamicEntryPlugin'. I'd like to fix that in this PR if possible or I will do it later. |
||
}) | ||
} | ||
|
||
function disposeInactiveEntries (devMiddleware, entries, lastAccessPages, maxInactiveAge) { | ||
const disposingPages = [] | ||
|
||
Object.keys(entries).forEach((page) => { | ||
const { lastActiveTime, status } = entries[page] | ||
|
||
// This means this entry is currently building or just added | ||
// We don't need to dispose those entries. | ||
if (status !== BUILT) return | ||
|
||
// We should not build the last accessed page even we didn't get any pings | ||
// Sometimes, it's possible our XHR ping to wait before completing other requests. | ||
// In that case, we should not dispose the current viewing page | ||
if (lastAccessPages[0] === page) return | ||
|
||
if (Date.now() - lastActiveTime > maxInactiveAge) { | ||
disposingPages.push(page) | ||
} | ||
}) | ||
|
||
if (disposingPages.length > 0) { | ||
disposingPages.forEach((page) => { | ||
delete entries[page] | ||
}) | ||
console.log(`> Disposing inactive page(s): ${disposingPages.join(', ')}`) | ||
devMiddleware.invalidate() | ||
} | ||
} | ||
|
||
// /index and / is the same. So, we need to identify both pages as the same. | ||
// This also applies to sub pages as well. | ||
function normalizePage (page) { | ||
return page.replace(/\/index$/, '/') | ||
} | ||
|
||
function sendJson (res, payload) { | ||
res.setHeader('Content-Type', 'application/json') | ||
res.status = 200 | ||
res.end(JSON.stringify(payload)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other places test with
process.env.NODE_ENV !== 'production'
instead.Developers might have forgotten to define
process.env.NODE_ENV
but still expect to have a development behaviour in that case.I think it would be nice if all the code tested whether it's in a production mode or not, in a coherent way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could use the
dev
provided when setting up the app (server side). Not sure if we pass it through to the client side though, will allow for consistent behaviour not based on NODE_ENV.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sedubois we set NODE_ENV always. See: https://github.com/zeit/next.js/pull/1111/files#diff-0f2f34c098f5954f99483c9bd61e439dR94
We didn't use 'production' check here because, we need to allow prefetching in JEST.
So, this is safe.