From a413d70313859f214c3f742b020d0a3a8a6af86d Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 10 Apr 2017 10:30:21 +0100 Subject: [PATCH] Asset amends (#8294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #8221 🔥 Remove ghost=true concept from asset url helper ✨ 💯 Introduce CSS minification with cssnano - add new grunt-cssnano dependency - wire up grunt task to minify public/ghost.css 🎨 Rename minification config & hash params - Change minifyInProduction -> hasMinFile - this means this asset should have a .min file available - Change minifyAssets -> useMinFiles - this means that in this env we want to serve .min files if available 🎨 Update public/ghost.css to serve .min for prod - add the new `hasMinFile` property 🎨 Move minified asset handling to asset_url util - this logic should be in the util, not the asset helper - updated tests 📖 Error handler always needs asset helper - this removes the TODO and adds a more sensible comment - we also need to update our theme documentation around error templates 🔥 Don't use asset helper in ghost head - use getAssetUrl util instead! - removed TODO 📖 Update proxy docs 🎨 Simplify asset helper & add tests - this refactor is a step prior to moving this from metadata to being a url util - needed to skip some new tests 🐛 Add missing handler for css file --- .gitignore | 1 + Gruntfile.js | 13 +- .../private-blogging/lib/views/private.hbs | 2 +- .../apps/subscribers/lib/views/subscribe.hbs | 2 +- core/server/blog/app.js | 1 + core/server/config/defaults.json | 2 + .../server/config/env/config.development.json | 2 +- .../config/env/config.testing-mysql.json | 2 +- core/server/config/env/config.testing.json | 2 +- core/server/data/meta/asset_url.js | 65 +-- core/server/helpers/asset.js | 16 +- core/server/helpers/ghost_head.js | 8 +- core/server/helpers/proxy.js | 1 - core/server/middleware/error-handler.js | 5 +- core/server/views/error.hbs | 2 +- core/test/unit/metadata/asset_url_spec.js | 147 ++++- core/test/unit/server_helpers/asset_spec.js | 97 +--- package.json | 1 + yarn.lock | 517 +++++++++++++++++- 19 files changed, 719 insertions(+), 167 deletions(-) diff --git a/.gitignore b/.gitignore index 7c34e83f53..b7257800ba 100644 --- a/.gitignore +++ b/.gitignore @@ -69,6 +69,7 @@ config.*.json /core/built /core/server/admin/views/* /core/server/public/ghost-url.min.js +/core/server/public/ghost.min.css # Coverage reports coverage.html diff --git a/Gruntfile.js b/Gruntfile.js index c99c914b4c..63f63bb840 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -323,6 +323,17 @@ var overrides = require('./core/server/overrides'), } }, + cssnano: { + prod: { + options: { + sourcemap: false + }, + files: { + 'core/server/public/ghost.min.css': 'core/server/public/ghost.css' + } + } + }, + // ### grunt-subgrunt // Run grunt tasks in submodule Gruntfiles subgrunt: { @@ -662,7 +673,7 @@ var overrides = require('./core/server/overrides'), // // It is otherwise the same as running `grunt`, but is only used when running Ghost in the `production` env. grunt.registerTask('prod', 'Build JS & templates for production', - ['subgrunt:prod', 'uglify:prod', 'master-warn']); + ['subgrunt:prod', 'uglify:prod', 'cssnano:prod', 'master-warn']); // ### Live reload // `grunt dev` - build assets on the fly whilst developing diff --git a/core/server/apps/private-blogging/lib/views/private.hbs b/core/server/apps/private-blogging/lib/views/private.hbs index c3c198f734..fbc19e267c 100644 --- a/core/server/apps/private-blogging/lib/views/private.hbs +++ b/core/server/apps/private-blogging/lib/views/private.hbs @@ -15,7 +15,7 @@ - +
diff --git a/core/server/apps/subscribers/lib/views/subscribe.hbs b/core/server/apps/subscribers/lib/views/subscribe.hbs index 6edfca9e57..4a3e8391dc 100644 --- a/core/server/apps/subscribers/lib/views/subscribe.hbs +++ b/core/server/apps/subscribers/lib/views/subscribe.hbs @@ -15,7 +15,7 @@ - +
diff --git a/core/server/blog/app.js b/core/server/blog/app.js index 8ed590bdc4..51b95b1a0a 100644 --- a/core/server/blog/app.js +++ b/core/server/blog/app.js @@ -53,6 +53,7 @@ module.exports = function setupBlogApp() { // Serve stylesheets for default templates blogApp.use(servePublicFile('public/ghost.css', 'text/css', utils.ONE_HOUR_S)); + blogApp.use(servePublicFile('public/ghost.min.css', 'text/css', utils.ONE_HOUR_S)); // Serve images for default templates blogApp.use(servePublicFile('public/404-ghost@2x.png', 'png', utils.ONE_HOUR_S)); diff --git a/core/server/config/defaults.json b/core/server/config/defaults.json index 984d8e0232..5114abc5a9 100644 --- a/core/server/config/defaults.json +++ b/core/server/config/defaults.json @@ -5,6 +5,8 @@ "port": 2368 }, "privacy": false, + "useMinFiles": true, + "printErrorStack": false, "paths": { "contentPath": "content/" }, diff --git a/core/server/config/env/config.development.json b/core/server/config/env/config.development.json index 3a6bf0ad1c..b3ac42cb65 100644 --- a/core/server/config/env/config.development.json +++ b/core/server/config/env/config.development.json @@ -17,7 +17,7 @@ "useRpcPing": false, "useUpdateCheck": true }, - "minifyAssets": false, + "useMinFiles": false, "printErrorStack": true, "caching": { "theme": { diff --git a/core/server/config/env/config.testing-mysql.json b/core/server/config/env/config.testing-mysql.json index b44090e016..2518e21aa2 100644 --- a/core/server/config/env/config.testing-mysql.json +++ b/core/server/config/env/config.testing-mysql.json @@ -53,5 +53,5 @@ "useTinfoil": true, "useStructuredData": true }, - "minifyAssets": false + "useMinFiles": false } diff --git a/core/server/config/env/config.testing.json b/core/server/config/env/config.testing.json index b0ae6e6003..f4f37533c4 100644 --- a/core/server/config/env/config.testing.json +++ b/core/server/config/env/config.testing.json @@ -52,5 +52,5 @@ "useTinfoil": true, "useStructuredData": true }, - "minifyAssets": false + "useMinFiles": false } diff --git a/core/server/data/meta/asset_url.js b/core/server/data/meta/asset_url.js index e3bbf50b8b..ddfb21e1df 100644 --- a/core/server/data/meta/asset_url.js +++ b/core/server/data/meta/asset_url.js @@ -2,50 +2,51 @@ var config = require('../../config'), settingsCache = require('../../settings/cache'), utils = require('../../utils'); -function getAssetUrl(path, isAdmin, minify) { - var output = ''; +/** + * Serve either uploaded favicon or default + * @return {string} + */ +function getFaviconUrl() { + if (settingsCache.get('icon')) { + return utils.url.urlJoin(utils.url.getSubdir(), utils.url.urlFor('image', {image: settingsCache.get('icon')})); + } - output += utils.url.urlJoin(utils.url.getSubdir(), '/'); + return utils.url.urlJoin(utils.url.getSubdir(), '/favicon.ico'); +} - if (!path.match(/^favicon\.(ico|png)$/) && !path.match(/^public/) && !path.match(/^asset/)) { - if (isAdmin) { - output = utils.url.urlJoin(output, 'ghost/'); - } +function getAssetUrl(path, hasMinFile) { + // CASE: favicon - this is special path with its own functionality + if (path.match(/\/?favicon\.(ico|png)$/)) { + // @TODO, resolve this - we should only be resolving subdirectory and extension. + return getFaviconUrl(); + } + // CASE: Build the output URL + // Add subdirectory... + var output = utils.url.urlJoin(utils.url.getSubdir(), '/'); + + // Optionally add /assets/ + if (!path.match(/^public/) && !path.match(/^asset/)) { output = utils.url.urlJoin(output, 'assets/'); } - // Serve either uploaded favicon or default - // for favicon, we don't care anymore about the `/` leading slash, as we don't support theme favicons - if (path.match(/\/?favicon\.(ico|png)$/)) { - if (isAdmin) { - output = utils.url.urlJoin(utils.url.getSubdir(), '/favicon.ico'); - } else { - if (settingsCache.get('icon')) { - output = utils.url.urlJoin(utils.url.getSubdir(), utils.url.urlFor('image', {image: settingsCache.get('icon')})); - } else { - output = utils.url.urlJoin(utils.url.getSubdir(), '/favicon.ico'); - } - } - } - // Get rid of any leading slash on the path - path = path.replace(/^\//, ''); - // replace ".foo" with ".min.foo" in production - if (minify) { + // replace ".foo" with ".min.foo" if configured + if (hasMinFile && config.get('useMinFiles') !== false) { path = path.replace(/\.([^\.]*)$/, '.min.$1'); } - if (!path.match(/^favicon\.(ico|png)$/)) { - // we don't want to concat the path with our favicon url - output += path; + // Add the path for the requested asset + output = utils.url.urlJoin(output, path); - if (!config.get('assetHash')) { - config.set('assetHash', utils.generateAssetHash()); - } - - output = output + '?v=' + config.get('assetHash'); + // Ensure we have an assetHash + // @TODO rework this! + if (!config.get('assetHash')) { + config.set('assetHash', utils.generateAssetHash()); } + // Finally add the asset hash to the output URL + output += '?v=' + config.get('assetHash'); + return output; } diff --git a/core/server/helpers/asset.js b/core/server/helpers/asset.js index 62c4a6fcee..f97eba14ec 100644 --- a/core/server/helpers/asset.js +++ b/core/server/helpers/asset.js @@ -3,24 +3,14 @@ // // Returns the path to the specified asset. The ghost flag outputs the asset path for the Ghost admin var proxy = require('./proxy'), - config = proxy.config, + _ = require('lodash'), getAssetUrl = proxy.metaData.getAssetUrl, SafeString = proxy.SafeString; module.exports = function asset(path, options) { - var isAdmin, - minify; - - if (options && options.hash) { - isAdmin = options.hash.ghost; - minify = options.hash.minifyInProduction; - } - - if (config.get('minifyAssets') === false) { - minify = false; - } + var hasMinFile = _.get(options, 'hash.hasMinFile'); return new SafeString( - getAssetUrl(path, isAdmin, minify) + getAssetUrl(path, hasMinFile) ); }; diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js index 1be48f037a..c19be9b9dc 100644 --- a/core/server/helpers/ghost_head.js +++ b/core/server/helpers/ghost_head.js @@ -11,6 +11,7 @@ var proxy = require('./proxy'), Promise = require('bluebird'), getMetaData = proxy.metaData.get, + getAssetUrl = proxy.metaData.getAssetUrl, escapeExpression = proxy.escapeExpression, SafeString = proxy.SafeString, filters = proxy.filters, @@ -66,12 +67,9 @@ function finaliseStructuredData(metaData) { } function getAjaxHelper(clientId, clientSecret) { - // @TODO: swap this for a direct utility, rather than using the helper? see #8221 - // Note: this is here because the asset helper isn't registered when this file is first loaded - var assetHelper = proxy.hbs.handlebars.helpers.asset; - return '\n' + + getAssetUrl('public/ghost-url.js', true) + + '">\n' + '