From b559938471baa6873cc98c0222d7e822cc9ae4f8 Mon Sep 17 00:00:00 2001 From: Sag Date: Mon, 6 Jul 2026 11:34:30 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20post=20card=20styles=20a?= =?UTF-8?q?nd=20scripts=20going=20missing=20until=20Ghost=20restarts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/ghost/issue/ONC-1865 - cards.min.css/js were built into content/public and served by reading them back from disk, so a wiped or unwritable content folder meant a 404 on every request until reboot - the assets are rebuilt from theme sources on every boot anyway, so the disk copy is not a real cache; the serving middleware already held the body in memory permanently - card assets are now kept in memory by the assets service and served from there; the disk write is best-effort back-compat only, so write failures and deleted files can no longer take the assets offline - build policy (single-flight, retry backoff, rebuild-on-theme-change) now lives in one place on the service instead of per-route Co-Authored-By: Claude Fable 5 --- .../assets-minification-base.js | 90 ++++++- .../assets-minification/card-assets.js | 37 ++- .../services/assets-minification/minifier.js | 79 +++++-- .../frontend/web/routers/serve-public-file.js | 72 +++++- .../assets-minification-base.test.js | 221 ++++++++++++++---- .../frontend/services/card-assets.test.js | 65 ++++++ .../web/middleware/serve-public-file.test.js | 176 +++++++++++++- 7 files changed, 659 insertions(+), 81 deletions(-) diff --git a/ghost/core/core/frontend/services/assets-minification/assets-minification-base.js b/ghost/core/core/frontend/services/assets-minification/assets-minification-base.js index eafd292e269..0b912a85282 100644 --- a/ghost/core/core/frontend/services/assets-minification/assets-minification-base.js +++ b/ghost/core/core/frontend/services/assets-minification/assets-minification-base.js @@ -1,19 +1,50 @@ const errors = require('@tryghost/errors'); const logging = require('@tryghost/logging'); +// Minimum time between builds triggered by requests. A build that keeps +// failing (or that never produces the requested file) must not re-run for +// every incoming request. invalidate() resets the clock so config changes +// (e.g. theme activation) always rebuild immediately. +const BUILD_MIN_INTERVAL_MS = 10000; + module.exports = class AssetsMinificationBase { minifier; ready = false; loading = null; + /** + * Incremented on invalidate() so an in-flight build can detect that its + * config went stale mid-build and re-run with the new config. + * @private + */ + generation = 0; + + /** + * Timestamp (ms) of when the last build settled — used for retry backoff. + * @private + */ + lastBuildSettledAt = 0; + + /** + * The error from the last failed build, if any. + * @private + */ + lastBuildError = null; + constructor(options = {}) { this.options = options; } invalidate() { + this.generation += 1; this.ready = false; - this.loading = null; + // Deliberately do NOT clear this.loading here: an in-flight build must + // not be forgotten, otherwise a new build could start while the old one + // is still running and two minifier runs would execute concurrently. + // ensureLoaded() re-runs load() when the generation changed mid-build. + this.lastBuildSettledAt = 0; + this.lastBuildError = null; } generateGlobs() { @@ -28,6 +59,48 @@ module.exports = class AssetsMinificationBase { }); } + /** + * Ensure the assets have been built. + * + * - Joins an in-flight build rather than starting a second one + * - Applies a retry backoff so a persistently failing (or non-producing) + * build doesn't run once per request + * - Re-runs the build when invalidate() was called mid-build, so the + * result always reflects the latest config + * + * @returns {Promise} resolves when a build has settled — rejects + * with the build error when the build failed (or recently failed and + * we're inside the backoff window) + */ + ensureLoaded() { + if (this.loading) { + return this.loading; + } + + if (this.lastBuildSettledAt && Date.now() - this.lastBuildSettledAt < BUILD_MIN_INTERVAL_MS) { + return this.lastBuildError ? Promise.reject(this.lastBuildError) : Promise.resolve(); + } + + const pending = (async () => { + let generation; + do { + generation = this.generation; + await this.load(); + } while (generation !== this.generation); + this.lastBuildError = null; + })().catch((error) => { + this.lastBuildError = error; + throw error; + }).finally(() => { + this.lastBuildSettledAt = Date.now(); + if (this.loading === pending) { + this.loading = null; + } + }); + this.loading = pending; + return pending; + } + async minify(globs, options) { try { const result = await this.minifier.minify(globs, options); @@ -52,15 +125,14 @@ module.exports = class AssetsMinificationBase { */ return async function serveMiddleware(req, res, next) { if (!self.ready) { - if (!self.loading) { - const pending = self.load().finally(() => { - if (self.loading === pending) { - self.loading = null; - } - }); - self.loading = pending; + try { + await self.ensureLoaded(); + } catch (error) { + // A failed build must not block the request — the rejection + // would escape the async handler and hang the request under + // Express 4. + logging.error(error); } - await self.loading; } next(); diff --git a/ghost/core/core/frontend/services/assets-minification/card-assets.js b/ghost/core/core/frontend/services/assets-minification/card-assets.js index a5201d59ac9..7d60a936b32 100644 --- a/ghost/core/core/frontend/services/assets-minification/card-assets.js +++ b/ghost/core/core/frontend/services/assets-minification/card-assets.js @@ -1,6 +1,7 @@ const debug = require('@tryghost/debug')('card-assets'); const _ = require('lodash'); const path = require('path'); +const logging = require('@tryghost/logging'); const config = require('../../../shared/config'); const Minifier = require('./minifier'); const AssetsMinificationBase = require('./assets-minification-base'); @@ -18,6 +19,13 @@ module.exports = class CardAssets extends AssetsMinificationBase { } this.files = []; + + /** + * Minified card assets, keyed by destination file name + * (e.g. `{'cards.min.css': '...'}`) — the source of truth for serving. + * @type {Object} + */ + this.outputs = {}; } /** @@ -62,6 +70,16 @@ module.exports = class CardAssets extends AssetsMinificationBase { return Object.keys(this.generateGlobs()).indexOf(`cards.min.${type}`) > -1; } + /** + * Get the minified contents of a built card asset + * + * @param {string} filename e.g. 'cards.min.css' + * @returns {string|null} the minified contents, or null when not built/produced + */ + getContent(filename) { + return this.outputs[filename] ?? null; + } + invalidate(cardAssetConfig) { if (cardAssetConfig) { this.config = cardAssetConfig; @@ -86,6 +104,23 @@ module.exports = class CardAssets extends AssetsMinificationBase { debug('globs', globs); - this.files = await this.minify(globs) || []; + // Build in memory — this is the source of truth for serving, so a + // missing or unwritable content folder can never take the assets + // offline. An in-memory build cannot fail with EACCES/ENOENT on the + // destination, so a successful build is always servable. + this.outputs = await this.minifier.minifyInMemory(globs); + this.files = Object.keys(this.outputs); + this.ready = true; + + // Best-effort disk write for back-compat with setups that expect the + // built files in content/public — failures are logged and must never + // affect serving. + for (const [dest, contents] of Object.entries(this.outputs)) { + try { + await this.minifier.writeFile(contents, dest); + } catch (error) { + logging.warn(`Ghost was not able to write card asset ${dest} to disk — serving it from memory. Reason: ${error.message}`); + } + } } }; diff --git a/ghost/core/core/frontend/services/assets-minification/minifier.js b/ghost/core/core/frontend/services/assets-minification/minifier.js index 302544d6770..42dae7fad3e 100644 --- a/ghost/core/core/frontend/services/assets-minification/minifier.js +++ b/ghost/core/core/frontend/services/assets-minification/minifier.js @@ -135,6 +135,39 @@ class Minifier { } } + /** + * Run the minification pipeline for a single destination: + * glob + concat the source files, apply replacements, minify. + * + * @private + * @param {string} src source glob + * @param {string} dest destination file name (must end in .css or .js) + * @param {Object} [options] + * @param {Object} [options.replacements] Key value pairs that should get replaced in the content before minifying + * @returns {Promise} Minified contents, or null when there was no output + */ + async minifyDestination(src, dest, options) { + let contents = await this.getSrcFileContents(src); + + if (options?.replacements) { + for (const key of Object.keys(options.replacements)) { + contents = contents.replace(key, options.replacements[key]); + } + } + + if (dest.endsWith('.css')) { + return await this.minifyCSS(contents); + } else if (dest.endsWith('.js')) { + return await this.minifyJS(contents); + } + + throw new errors.IncorrectUsageError({ + message: tpl(messages.badDestination.message, {dest}), + context: tpl(messages.badDestination.context), + help: tpl(messages.globalHelp) + }); + } + /** * Minify files * @@ -155,27 +188,7 @@ class Minifier { const minifiedFiles = []; for (const dest of destinations) { - const src = globs[dest]; - let contents = await this.getSrcFileContents(src); - - if (options?.replacements) { - for (const key of Object.keys(options.replacements)) { - contents = contents.replace(key, options.replacements[key]); - } - } - let minifiedContents; - - if (dest.endsWith('.css')) { - minifiedContents = await this.minifyCSS(contents); - } else if (dest.endsWith('.js')) { - minifiedContents = await this.minifyJS(contents); - } else { - throw new errors.IncorrectUsageError({ - message: tpl(messages.badDestination.message, {dest}), - context: tpl(messages.badDestination.context), - help: tpl(messages.globalHelp) - }); - } + const minifiedContents = await this.minifyDestination(globs[dest], dest, options); const result = await this.writeFile(minifiedContents, dest); if (result) { @@ -186,6 +199,30 @@ class Minifier { debug('End'); return minifiedFiles; } + + /** + * Minify files in memory, without writing anything to disk + * + * @param {Object} globs Same shape as minify() + * @param {Object} [options] + * @param {Object} [options.replacements] Key value pairs that should get replaced in the content before minifying + * @returns {Promise>} Map of destination file name to minified contents + * (destinations whose minified output was empty are omitted) + */ + async minifyInMemory(globs, options) { + debug('Begin (in memory)', globs); + const outputs = {}; + + for (const dest of Object.keys(globs)) { + const minifiedContents = await this.minifyDestination(globs[dest], dest, options); + if (minifiedContents) { + outputs[dest] = minifiedContents; + } + } + + debug('End (in memory)'); + return outputs; + } } module.exports = Minifier; diff --git a/ghost/core/core/frontend/web/routers/serve-public-file.js b/ghost/core/core/frontend/web/routers/serve-public-file.js index c1a6797a878..94f650bce59 100644 --- a/ghost/core/core/frontend/web/routers/serve-public-file.js +++ b/ghost/core/core/frontend/web/routers/serve-public-file.js @@ -2,6 +2,7 @@ const crypto = require('crypto'); const fs = require('fs-extra'); const path = require('path'); const errors = require('@tryghost/errors'); +const logging = require('@tryghost/logging'); const config = require('../../../shared/config'); const urlUtils = require('../../../shared/url-utils'); const tpl = require('@tryghost/tpl'); @@ -102,6 +103,70 @@ function createPublicFileMiddleware(location, file, mime, maxAge, options = {}) }; } +/** + * Serve an asset straight from an in-memory assets service (e.g. card assets) + * instead of reading it back from disk. The service is the source of truth: + * a wiped or unwritable content folder can never 404 these assets. + * + * @param {Object} service An assets-minification service exposing `ready`, + * `ensureLoaded()` and `getContent(filename)` + * @param {string} filename e.g. 'cards.min.css' + * @param {string} mime + * @param {number} maxAge + */ +function createInMemoryAssetMiddleware(service, filename, mime, maxAge) { + let cache = null; + const blogRegex = /(\{\{blog-url\}\})/g; + + return async function serveInMemoryAssetMiddleware(req, res, next) { + if (!service.ready) { + try { + await service.ensureLoaded(); + } catch (error) { + // A failed build must not block the request — the rejection + // would escape the async handler and hang the request under + // Express 4. If there's no content we 404 below; the service + // retries the build on a later request (past its backoff). + logging.error(error); + } + } + + const content = service.getContent(filename); + + if (content === null || content === undefined) { + return next(new errors.NotFoundError({ + message: tpl(messages.fileNotFound), + code: 'PUBLIC_FILE_NOT_FOUND' + })); + } + + // Memoize the computed body + headers keyed by the content reference, + // so the md5 isn't recomputed per request. A rebuild produces a new + // string reference, which invalidates the memo. + if (!cache || cache.content !== content) { + let str = content; + + if (mime === 'text/xsl' || mime === 'text/plain' || mime === 'application/javascript') { + str = str.replace(blogRegex, urlUtils.urlFor('home', true).replace(/\/$/, '')); + } + + cache = { + content, + headers: { + 'Content-Type': mime, + 'Content-Length': Buffer.from(str).length, + ETag: `"${crypto.createHash('md5').update(str, 'utf8').digest('hex')}"`, + 'Cache-Control': `public, max-age=${maxAge}` + }, + body: str + }; + } + + res.writeHead(200, cache.headers); + res.end(cache.body); + }; +} + // Handles requests to robots.txt and favicon.ico (and caches them) function servePublicFile(location, file, type, maxAge, options = {}) { const publicFileMiddleware = createPublicFileMiddleware(location, file, type, maxAge, options); @@ -127,9 +192,9 @@ function servePublicFiles(siteApp) { // Traffic analytics tracking script siteApp.get('/public/ghost-stats.min.js', createPublicFileMiddleware('static', 'public/ghost-stats.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge'))); - // Card assets (built on the fly) - siteApp.get('/public/cards.min.css', cardAssets.serveMiddleware(), createPublicFileMiddleware('built', 'public/cards.min.css', 'text/css', config.get('caching:publicAssets:maxAge'))); - siteApp.get('/public/cards.min.js', cardAssets.serveMiddleware(), createPublicFileMiddleware('built', 'public/cards.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge'))); + // Card assets (built on the fly, served from memory) + siteApp.get('/public/cards.min.css', createInMemoryAssetMiddleware(cardAssets, 'cards.min.css', 'text/css', config.get('caching:publicAssets:maxAge'))); + siteApp.get('/public/cards.min.js', createInMemoryAssetMiddleware(cardAssets, 'cards.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge'))); // Comment counts siteApp.get('/public/comment-counts.min.js', createPublicFileMiddleware('static', 'public/comment-counts.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge'))); @@ -163,3 +228,4 @@ function servePublicFiles(siteApp) { module.exports = servePublicFiles; module.exports.servePublicFile = servePublicFile; module.exports.createPublicFileMiddleware = createPublicFileMiddleware; +module.exports.createInMemoryAssetMiddleware = createInMemoryAssetMiddleware; diff --git a/ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js b/ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js index 994bc85eaaa..2ac2658e38b 100644 --- a/ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js +++ b/ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js @@ -3,6 +3,7 @@ const sinon = require('sinon'); const path = require('path'); const fs = require('fs').promises; const os = require('os'); +const logging = require('@tryghost/logging'); const AssetsMinificationBase = require('../../../../../core/frontend/services/assets-minification/assets-minification-base'); describe('AssetsMinificationBase', function () { @@ -20,6 +21,172 @@ describe('AssetsMinificationBase', function () { sinon.restore(); }); + describe('ensureLoaded', function () { + it('joins an in-flight build instead of starting a second one', async function () { + let loadCallCount = 0; + let resolveLoad; + + class TestAssets extends AssetsMinificationBase { + async load() { + loadCallCount += 1; + await new Promise((resolve) => { + resolveLoad = resolve; + }); + this.ready = true; + } + } + + const assets = new TestAssets(); + + const first = assets.ensureLoaded(); + const second = assets.ensureLoaded(); + + assert.equal(first, second, 'both callers should get the same promise'); + + resolveLoad(); + await Promise.all([first, second]); + + assert.equal(loadCallCount, 1); + assert.equal(assets.ready, true); + assert.equal(assets.loading, null); + }); + + it('does not rebuild within the backoff window after a failure and rejects with the stored error', async function () { + let loadCallCount = 0; + const buildError = new Error('build failed'); + + class TestAssets extends AssetsMinificationBase { + async load() { + loadCallCount += 1; + throw buildError; + } + } + + const assets = new TestAssets(); + + await assert.rejects(() => assets.ensureLoaded(), buildError); + assert.equal(loadCallCount, 1); + assert.equal(assets.loading, null); + + // Second call within the backoff window: no new build, same error + await assert.rejects(() => assets.ensureLoaded(), buildError); + assert.equal(loadCallCount, 1, 'load() should not run again within the backoff window'); + }); + + it('does not rebuild within the backoff window after a build that produced nothing', async function () { + let loadCallCount = 0; + + class TestAssets extends AssetsMinificationBase { + async load() { + // e.g. theme config asked for no assets: build succeeds + // but never sets ready via minify() + loadCallCount += 1; + } + } + + const assets = new TestAssets(); + + await assets.ensureLoaded(); + assert.equal(loadCallCount, 1); + + // Resolves without starting another build + await assets.ensureLoaded(); + assert.equal(loadCallCount, 1, 'load() should not run again within the backoff window'); + }); + + it('rebuilds after the backoff window has passed', async function () { + const clock = sinon.useFakeTimers({now: Date.now(), toFake: ['Date']}); + let loadCallCount = 0; + + class TestAssets extends AssetsMinificationBase { + async load() { + loadCallCount += 1; + throw new Error(`build failed ${loadCallCount}`); + } + } + + const assets = new TestAssets(); + + await assert.rejects(() => assets.ensureLoaded(), /build failed 1/); + assert.equal(loadCallCount, 1); + + clock.tick(10001); + + await assert.rejects(() => assets.ensureLoaded(), /build failed 2/); + assert.equal(loadCallCount, 2, 'load() should run again after the backoff window'); + }); + + it('invalidate() resets the backoff so a config change rebuilds immediately', async function () { + let loadCallCount = 0; + + class TestAssets extends AssetsMinificationBase { + async load() { + loadCallCount += 1; + if (loadCallCount === 1) { + throw new Error('build failed'); + } + this.ready = true; + } + } + + const assets = new TestAssets(); + + await assert.rejects(() => assets.ensureLoaded(), /build failed/); + assert.equal(loadCallCount, 1); + + assets.invalidate(); + + await assets.ensureLoaded(); + assert.equal(loadCallCount, 2, 'load() should run immediately after invalidate()'); + assert.equal(assets.ready, true); + }); + + it('re-runs load() when invalidate() is called mid-build, without concurrent loads', async function () { + let loadCallCount = 0; + let concurrentLoads = 0; + let maxConcurrentLoads = 0; + let resolveFirstLoad; + + class TestAssets extends AssetsMinificationBase { + async load() { + loadCallCount += 1; + concurrentLoads += 1; + maxConcurrentLoads = Math.max(maxConcurrentLoads, concurrentLoads); + try { + if (loadCallCount === 1) { + await new Promise((resolve) => { + resolveFirstLoad = resolve; + }); + } + this.ready = true; + } finally { + concurrentLoads -= 1; + } + } + } + + const assets = new TestAssets(); + + // First build starts and hangs + const first = assets.ensureLoaded(); + + // Theme is activated mid-build: config changed, build result is stale + assets.invalidate(); + + // A second caller must join the in-flight build, not start a new one + const second = assets.ensureLoaded(); + assert.equal(first, second, 'in-flight build must not be forgotten by invalidate()'); + + resolveFirstLoad(); + await Promise.all([first, second]); + + assert.equal(loadCallCount, 2, 'load() should re-run with the new config'); + assert.equal(maxConcurrentLoads, 1, 'two loads must never run concurrently'); + assert.equal(assets.ready, true); + assert.equal(assets.loading, null); + }); + }); + describe('serveMiddleware', function () { it('calls load() only once when multiple requests arrive concurrently', async function () { let loadCallCount = 0; @@ -139,44 +306,6 @@ describe('AssetsMinificationBase', function () { assert.equal(loadCallCount, 2); }); - it('does not clobber a new loading promise when invalidate() is called mid-flight', async function () { - let loadCallCount = 0; - let resolveFirstLoad; - - class TestAssets extends AssetsMinificationBase { - async load() { - loadCallCount += 1; - if (loadCallCount === 1) { - await new Promise((resolve) => { - resolveFirstLoad = resolve; - }); - } - this.ready = true; - } - } - - const assets = new TestAssets(); - const middleware = assets.serveMiddleware(); - const next = sinon.stub(); - - // First request starts load, which hangs - const firstRequest = middleware({}, {}, next); - - // invalidate() mid-flight: clears loading and ready - assets.invalidate(); - - // Second request starts a new load since loading was cleared - const secondRequest = middleware({}, {}, next); - - // First load settles — its .finally() must NOT clobber the second load - resolveFirstLoad(); - await firstRequest; - await secondRequest; - - assert.equal(loadCallCount, 2, 'load() should be called twice (once per invalidation cycle)'); - sinon.assert.calledTwice(next); - }); - it('clears loading promise after load() completes', async function () { class TestAssets extends AssetsMinificationBase { async load() { @@ -193,16 +322,13 @@ describe('AssetsMinificationBase', function () { assert.equal(assets.loading, null); }); - it('clears loading promise even when load() throws', async function () { - let shouldThrow = true; + it('continues the request (and logs) when load() rejects', async function () { + const loggingStub = sinon.stub(logging, 'error'); + const buildError = new Error('load failed'); class TestAssets extends AssetsMinificationBase { async load() { - if (shouldThrow) { - shouldThrow = false; - throw new Error('load failed'); - } - this.ready = true; + throw buildError; } } @@ -210,8 +336,11 @@ describe('AssetsMinificationBase', function () { const middleware = assets.serveMiddleware(); const next = sinon.stub(); - await assert.rejects(() => middleware({}, {}, next)); + // Must not reject — a rejection would hang the request under Express 4 + await middleware({}, {}, next); + sinon.assert.calledOnce(next); + sinon.assert.calledOnceWithExactly(loggingStub, buildError); assert.equal(assets.loading, null); }); }); diff --git a/ghost/core/test/unit/frontend/services/card-assets.test.js b/ghost/core/test/unit/frontend/services/card-assets.test.js index 491d0d37526..a0950506075 100644 --- a/ghost/core/test/unit/frontend/services/card-assets.test.js +++ b/ghost/core/test/unit/frontend/services/card-assets.test.js @@ -1,8 +1,10 @@ const assert = require('node:assert/strict'); +const sinon = require('sinon'); const path = require('path'); const fs = require('fs').promises; const os = require('os'); +const logging = require('@tryghost/logging'); const CardAssetService = require('../../../../core/frontend/services/assets-minification/card-assets'); @@ -28,6 +30,10 @@ describe('Card Asset Service', function () { await fs.rm(testDir, {recursive: true}); }); + afterEach(function () { + sinon.restore(); + }); + it('can load nothing', async function () { const cardAssets = new CardAssetService({ src: srcDir, @@ -37,6 +43,8 @@ describe('Card Asset Service', function () { await cardAssets.load(); assert.deepEqual(cardAssets.files, []); + assert.deepEqual(cardAssets.outputs, {}); + assert.equal(cardAssets.ready, true); }); it('can load a single css file', async function () { @@ -50,6 +58,7 @@ describe('Card Asset Service', function () { await cardAssets.load(true); assert.deepEqual(cardAssets.files, ['cards.min.css']); + assert.equal(cardAssets.ready, true); }); it('can correctly load nothing when config is false', async function () { @@ -63,6 +72,62 @@ describe('Card Asset Service', function () { await cardAssets.load(false); assert.deepEqual(cardAssets.files, []); + assert.deepEqual(cardAssets.outputs, {}); + }); + + it('keeps the minified contents in memory and exposes them via getContent', async function () { + const cardAssets = new CardAssetService({ + src: srcDir, + dest: destDir + }); + + await fs.writeFile(path.join(srcDir, 'css', 'test.css'), '.test { color: #fff }'); + await fs.writeFile(path.join(srcDir, 'js', 'test.js'), 'const test = "hello world";console.log(test);'); + + await cardAssets.load(true); + + assert.deepEqual(cardAssets.files.sort(), ['cards.min.css', 'cards.min.js']); + assert.match(cardAssets.getContent('cards.min.css'), /\.test\{color:#fff\}/); + assert.match(cardAssets.getContent('cards.min.js'), /hello world/); + assert.equal(cardAssets.getContent('unknown.min.js'), null); + assert.equal(cardAssets.hasFile('css'), true); + assert.equal(cardAssets.hasFile('js'), true); + }); + + it('writes the minified files to disk as a best-effort side effect', async function () { + const cardAssets = new CardAssetService({ + src: srcDir, + dest: destDir + }); + + await fs.writeFile(path.join(srcDir, 'css', 'test.css'), '.test { color: #fff }'); + + await cardAssets.load(true); + + const diskContents = await fs.readFile(path.join(destDir, 'cards.min.css'), 'utf8'); + assert.equal(diskContents, cardAssets.getContent('cards.min.css')); + }); + + it('is still ready + servable when the disk write fails', async function () { + const loggingStub = sinon.stub(logging, 'warn'); + + const cardAssets = new CardAssetService({ + src: srcDir, + dest: destDir + }); + + await fs.writeFile(path.join(srcDir, 'css', 'test.css'), '.test { color: #fff }'); + + const eaccesError = new Error('permission denied'); + eaccesError.code = 'EACCES'; + sinon.stub(cardAssets.minifier, 'writeFile').rejects(eaccesError); + + await cardAssets.load(true); + + assert.equal(cardAssets.ready, true, 'a failed disk write must not affect readiness'); + assert.match(cardAssets.getContent('cards.min.css'), /\.test\{color:#fff\}/); + assert.equal(cardAssets.hasFile('css'), true); + sinon.assert.called(loggingStub); }); describe('Generate the correct glob strings', function () { diff --git a/ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js b/ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js index 23fef549c50..d71e13e25e0 100644 --- a/ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js +++ b/ghost/core/test/unit/frontend/web/middleware/serve-public-file.test.js @@ -3,8 +3,9 @@ const sinon = require('sinon'); const request = require('supertest'); const express = require('express'); const fs = require('fs-extra'); +const logging = require('@tryghost/logging'); const config = require('../../../../../core/shared/config'); -const {servePublicFile} = require('../../../../../core/frontend/web/routers/serve-public-file'); +const {servePublicFile, createInMemoryAssetMiddleware} = require('../../../../../core/frontend/web/routers/serve-public-file'); describe('servePublicFile', function () { afterEach(function () { @@ -204,3 +205,176 @@ describe('servePublicFile', function () { assert(fileStub.firstCall.args[0].endsWith('core/frontend/public/private.min.js')); }); }); + +describe('createInMemoryAssetMiddleware', function () { + afterEach(function () { + sinon.restore(); + }); + + function createApp(middleware) { + const app = express(); + app.use(middleware); + app.use((_req, res) => { + res.status(418).send('next'); + }); + app.use((err, _req, res, _next) => { + void _next; + + res.status(err.statusCode || 500).json({ + errorType: err.errorType, + code: err.code + }); + }); + return app; + } + + function createService({outputs = {}, ready = true} = {}) { + const service = { + ready, + outputs, + ensureLoaded: sinon.stub().callsFake(async function () { + service.ready = true; + }), + getContent(filename) { + return service.outputs[filename] ?? null; + } + }; + return service; + } + + it('serves the content from the service with the correct headers', async function () { + const body = '.card{color:#fff}'; + const service = createService({outputs: {'cards.min.css': body}}); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + const {headers, text} = await request(app) + .get('/public/cards.min.css') + .expect(200); + + assert.equal(text, body); + assert.match(headers['content-type'], /^text\/css/); + assert.equal(headers['content-length'], `${Buffer.from(body).length}`); + assert.match(headers.etag, /^".+"$/); + assert.equal(headers['cache-control'], 'public, max-age=3600'); + sinon.assert.notCalled(service.ensureLoaded); + }); + + it('triggers a build when the service is not ready', async function () { + const service = createService({outputs: {}, ready: false}); + service.ensureLoaded.callsFake(async function () { + service.ready = true; + service.outputs = {'cards.min.js': 'console.log("built");'}; + }); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.js', 'application/javascript', 3600); + const app = createApp(middleware); + + const {text} = await request(app) + .get('/public/cards.min.js') + .expect(200); + + assert.equal(text, 'console.log("built");'); + sinon.assert.calledOnce(service.ensureLoaded); + }); + + it('replaces {{blog-url}} in javascript assets', async function () { + const service = createService({outputs: {'cards.min.js': 'const url = "{{blog-url}}";'}}); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.js', 'application/javascript', 3600); + const app = createApp(middleware); + + const siteUrl = config.get('url').replace(/\/$/, ''); + + await request(app) + .get('/public/cards.min.js') + .expect(200) + .expect(`const url = "${siteUrl}";`); + }); + + it('does not replace {{blog-url}} in css assets', async function () { + const service = createService({outputs: {'cards.min.css': '/* {{blog-url}} */'}}); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + await request(app) + .get('/public/cards.min.css') + .expect(200) + .expect('/* {{blog-url}} */'); + }); + + it('404s when the content is absent after a build', async function () { + const service = createService({outputs: {}, ready: false}); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + await request(app) + .get('/public/cards.min.css') + .expect(404) + .expect({ + errorType: 'NotFoundError', + code: 'PUBLIC_FILE_NOT_FOUND' + }); + + sinon.assert.calledOnce(service.ensureLoaded); + }); + + it('404s (and logs) instead of hanging when the build fails', async function () { + const loggingStub = sinon.stub(logging, 'error'); + const buildError = new Error('build failed'); + const service = createService({outputs: {}, ready: false}); + service.ensureLoaded.rejects(buildError); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + await request(app) + .get('/public/cards.min.css') + .expect(404) + .expect({ + errorType: 'NotFoundError', + code: 'PUBLIC_FILE_NOT_FOUND' + }); + + sinon.assert.calledOnceWithExactly(loggingStub, buildError); + }); + + it('still serves stale content when a rebuild fails', async function () { + sinon.stub(logging, 'error'); + const service = createService({outputs: {'cards.min.css': '.stale{}'}, ready: false}); + service.ensureLoaded.rejects(new Error('rebuild failed')); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + await request(app) + .get('/public/cards.min.css') + .expect(200) + .expect('.stale{}'); + }); + + it('serves updated content (with a new ETag) after a rebuild', async function () { + const service = createService({outputs: {'cards.min.css': '.one{}'}}); + const middleware = createInMemoryAssetMiddleware(service, 'cards.min.css', 'text/css', 3600); + const app = createApp(middleware); + + const firstResponse = await request(app) + .get('/public/cards.min.css') + .expect(200) + .expect('.one{}'); + + const secondResponse = await request(app) + .get('/public/cards.min.css') + .expect(200) + .expect('.one{}'); + + assert.equal(secondResponse.headers.etag, firstResponse.headers.etag); + + // Simulate a rebuild (e.g. theme change) producing new content + service.outputs = {'cards.min.css': '.two{}'}; + + const thirdResponse = await request(app) + .get('/public/cards.min.css') + .expect(200) + .expect('.two{}'); + + assert.notEqual(thirdResponse.headers.etag, firstResponse.headers.etag); + assert.equal(thirdResponse.headers['content-length'], `${Buffer.from('.two{}').length}`); + }); +});