Skip to content

Commit

Permalink
Refactored boot time / on demand asset generation to occur at build time
Browse files Browse the repository at this point in the history
refs [ONC-662](https://linear.app/ghost/issue/ONC-662/fix-file-write-issues-in-ghost-application-related-to-asset-generation)

Refactored the generation of the following assets:
- `comment-counts.js`
- `member-attribution.js`
- `admin-auth`

To occur at build time instead of at boot time or on demand

`cards` are still built on demand as these assets need to be re-generated
when a theme changes - we utilise the existing asset minification system for
this, except we only execute the middleware responsible for this when a request
is requesting a card asset

This also reverts the changes made in [#21857](#21857)
as these changes are no longer needed (because the assets are now generated at
build time)
  • Loading branch information
mike182uk committed Dec 12, 2024
1 parent e2fad8b commit e48ee21
Show file tree
Hide file tree
Showing 18 changed files with 89 additions and 239 deletions.
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ test/functional/*.png
# Built asset files
/ghost/core/core/built
/ghost/core/core/frontend/public/ghost.min.css
/ghost/core/core/frontend/public/comment-counts.min.js
/ghost/core/core/frontend/public/member-attribution.min.js
/ghost/core/core/frontend/public/admin-auth/admin-auth.min.js

# Caddyfile - for local development with ssl + caddy
Caddyfile
Expand Down
12 changes: 1 addition & 11 deletions ghost/core/core/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const logging = require('@tryghost/logging');
const tpl = require('@tryghost/tpl');
const themeEngine = require('./frontend/services/theme-engine');
const appService = require('./frontend/services/apps');
const {adminAuthAssets, cardAssets,commentCountsAssets, memberAttributionAssets} = require('./frontend/services/assets-minification');
const {cardAssets} = require('./frontend/services/assets-minification');
const routerManager = require('./frontend/services/routing').routerManager;
const settingsCache = require('./shared/settings-cache');
const urlService = require('./server/services/url');
Expand Down Expand Up @@ -51,10 +51,6 @@ class Bridge {
return themeEngine.getActive();
}

ensureAdminAuthAssetsMiddleware() {
return adminAuthAssets.serveMiddleware();
}

async activateTheme(loadedTheme, checkedTheme) {
let settings = {
locale: settingsCache.get('locale')
Expand All @@ -69,12 +65,6 @@ class Bridge {
const cardAssetConfig = this.getCardAssetConfig();
debug('reload card assets config', cardAssetConfig);
cardAssets.invalidate(cardAssetConfig);

// TODO: is this in the right place?
// rebuild asset files
commentCountsAssets.invalidate();
adminAuthAssets.invalidate();
memberAttributionAssets.invalidate();
} catch (err) {
logging.error(new errors.InternalServerError({
message: tpl(messages.activateFailed, {theme: loadedTheme.name}),
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

13 changes: 2 additions & 11 deletions ghost/core/core/frontend/services/assets-minification/index.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,7 @@
const AdminAuthAssets = require('./AdminAuthAssets');
const CardAssets = require('./CardAssets');
const CommentCountsAssets = require('./CommentCountsAssets');
const MemberAttributionAssets = require('./MemberAttributionAssets');

const adminAuthAssets = new AdminAuthAssets();
const cardAssets = new CardAssets();
const commentCountsAssets = new CommentCountsAssets();
const memberAttributionAssets = new MemberAttributionAssets();

module.exports = {
adminAuthAssets,
cardAssets,
commentCountsAssets,
memberAttributionAssets
};
cardAssets
};
10 changes: 0 additions & 10 deletions ghost/core/core/frontend/src/member-attribution/.eslintrc

This file was deleted.

30 changes: 25 additions & 5 deletions ghost/core/core/frontend/web/site.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const membersService = require('../../server/services/members');
const offersService = require('../../server/services/offers');
const customRedirects = require('../../server/services/custom-redirects');
const linkRedirects = require('../../server/services/link-redirection');
const {cardAssets, commentCountsAssets, memberAttributionAssets} = require('../services/assets-minification');
const {cardAssets} = require('../services/assets-minification');
const siteRoutes = require('./routes');
const shared = require('../../server/web/shared');
const errorHandler = require('@tryghost/mw-error-handler');
Expand Down Expand Up @@ -73,14 +73,34 @@ module.exports = function setupSiteApp(routerConfig) {
siteApp.use(mw.servePublicFile('static', 'public/ghost.min.css', 'text/css', config.get('caching:publicAssets:maxAge')));

// Card assets
siteApp.use(cardAssets.serveMiddleware(), mw.servePublicFile('built', 'public/cards.min.css', 'text/css', config.get('caching:publicAssets:maxAge')));
siteApp.use(cardAssets.serveMiddleware(), mw.servePublicFile('built', 'public/cards.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge')));
const cardsCssPath = 'public/cards.min.css';
const cardsJsPath = 'public/cards.min.js';
siteApp.use(
function serveCardsCSSMiddleware(req, res, next) {
if (req.path === `/${cardsCssPath}`) {
return cardAssets.serveMiddleware()(req, res, next);
}

return next();
},
mw.servePublicFile('built', cardsCssPath, 'text/css', config.get('caching:publicAssets:maxAge'))
);
siteApp.use(
function serveCardsJSMiddleware(req, res, next) {
if (req.path === `/${cardsJsPath}`) {
return cardAssets.serveMiddleware()(req, res, next);
}

return next();
},
mw.servePublicFile('built', cardsJsPath, 'application/javascript', config.get('caching:publicAssets:maxAge'))
);

// Comment counts
siteApp.use(commentCountsAssets.serveMiddleware(), mw.servePublicFile('built', 'public/comment-counts.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge')));
siteApp.use(mw.servePublicFile('static', 'public/comment-counts.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge')));

// Member attribution
siteApp.use(memberAttributionAssets.serveMiddleware(), mw.servePublicFile('built', 'public/member-attribution.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge')));
siteApp.use(mw.servePublicFile('static', 'public/member-attribution.min.js', 'application/javascript', config.get('caching:publicAssets:maxAge')));

// Serve site images using the storage adapter
siteApp.use(STATIC_IMAGE_URL_PREFIX, mw.handleImageSizes, storage.getStorage('images').serve());
Expand Down
8 changes: 3 additions & 5 deletions ghost/core/core/server/web/admin/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const shared = require('../shared');
const errorHandler = require('@tryghost/mw-error-handler');
const sentry = require('../../../shared/sentry');
const redirectAdminUrls = require('./middleware/redirect-admin-urls');
const bridge = require('../../../bridge');
const createServeAuthFrameFileMw = require('./middleware/serve-admin-auth-frame-file');

/**
*
Expand Down Expand Up @@ -39,7 +39,7 @@ module.exports = function setupAdminApp() {
// request to the Admin API /users/me/ endpoint to check if the user is logged in.
//
// Used by comments-ui to add moderation options to front-end comments when logged in.
adminApp.use('/auth-frame', bridge.ensureAdminAuthAssetsMiddleware(), function authFrameMw(req, res, next) {
adminApp.use('/auth-frame', function authFrameMw(req, res, next) {
// only render content when we have an Admin session cookie,
// otherwise return a 204 to avoid JS and API requests being made unnecessarily
try {
Expand All @@ -52,9 +52,7 @@ module.exports = function setupAdminApp() {
} catch (err) {
next(err);
}
}, serveStatic(
path.join(config.getContentPath('public'), 'admin-auth')
));
}, createServeAuthFrameFileMw(config, urlUtils));

// Ember CLI's live-reload script
if (config.get('env') === 'development') {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
const path = require('node:path');
const fs = require('node:fs/promises');

function createServeAuthFrameFileMw(config, urlUtils) {
const placeholders = {
'{{SITE_ORIGIN}}': new URL(urlUtils.getSiteUrl()).origin
};

return function serveAuthFrameFileMw(req, res, next) {
const filename = path.parse(req.url).base;
let basePath = path.join(config.get('paths').publicFilePath, 'admin-auth');
let filePath;

if (filename === '') {
filePath = path.join(basePath, 'index.html');
} else {
filePath = path.join(basePath, filename);
}

fs.readFile(filePath).then((data) => {
let dataString = data.toString();

for (const [key, value] of Object.entries(placeholders)) {
dataString = dataString.replace(key, value);
}

res.end(dataString);
}).catch((err) => {
next(err); // TODO
});
};
}

module.exports = createServeAuthFrameFileMw;
22 changes: 1 addition & 21 deletions ghost/core/core/shared/config/helpers.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const crypto = require('crypto');
const os = require('os');
const path = require('path');
const {URL} = require('url');

Expand Down Expand Up @@ -66,24 +64,6 @@ const isPrivacyDisabled = function isPrivacyDisabled(privacyFlag) {
return this.get('privacy')[privacyFlag] === false;
};

/** @type {string|null} */
let processTmpDirPath = null;

/**
* Get a tmp dir path for the current process
*
* @returns {string} - tmp dir path for the current process
*/
function getProcessTmpDirPath() {
// Memoize the computed path to avoid re-computing it on each call - The
// value should not change during the lifetime of the process.
if (processTmpDirPath === null) {
processTmpDirPath = path.join(os.tmpdir(), `ghost_${crypto.randomUUID()}`);
}

return processTmpDirPath;
}

/**
* @callback getContentPathFn
* @param {string} type - the type of context you want the path for
Expand All @@ -108,7 +88,7 @@ const getContentPath = function getContentPath(type) {
case 'settings':
return path.join(this.get('paths:contentPath'), 'settings/');
case 'public':
return path.join(getProcessTmpDirPath(this), 'public/');
return path.join(this.get('paths:contentPath'), 'public/');
default:
// new Error is allowed here, as we do not want config to depend on @tryghost/error
// @TODO: revisit this decision when @tryghost/error is no longer dependent on all of ghost-ignition
Expand Down
4 changes: 3 additions & 1 deletion ghost/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
"scripts": {
"archive": "npm pack",
"dev": "node --watch index.js",
"build:assets": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css",
"build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css",
"build:assets:js": "node ../minifier/bin/single.js core/frontend/public/comment-counts.js core/frontend/public/comment-counts.min.js && node ../minifier/bin/single.js core/frontend/public/member-attribution.js core/frontend/public/member-attribution.min.js && node ../minifier/bin/single.js core/frontend/public/admin-auth/message-handler.js core/frontend/public/admin-auth/admin-auth.min.js",
"build:assets": "yarn build:assets:css && yarn build:assets:js",
"test": "yarn test:unit",
"test:base": "mocha --reporter dot --require=./test/utils/overrides.js --exit --trace-warnings --recursive --extension=test.js",
"test:single": "yarn test:base --timeout=60000",
Expand Down
Loading

0 comments on commit e48ee21

Please sign in to comment.