From e9ae0d9baa104e205cb58c4012ba7176fe70ab69 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 2 Jul 2020 18:03:22 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20`srcset`=20and=20`sizes`?= =?UTF-8?q?=20attributes=20being=20rendered=20when=20image=20resizing=20un?= =?UTF-8?q?available?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Ghost/issues/11944 - updates `@tryghost/image-transform` to version that exposes `canTransformFiles()` which checks for `sharp` availibility - updates `@tryghost/kg-default-cards` to version that accepts a `canTransformImage()` method as an option - updates our `mobiledoc` lib to pass a `canTransformImage()` function that returns false if sharp is unavailable, the image extension is not supported, or the storage engine in use does not support image transforms - updates `populateImageSizes` to fetch image sizes when transforms are unavailable as the render/not-render is now handled in the renderer and we don't need to worry about adding size information to the mobiledoc source --- core/server/lib/mobiledoc.js | 22 +++--- .../web/site/middleware/handle-image-sizes.js | 5 ++ package.json | 4 +- test/unit/lib/mobiledoc_spec.js | 68 +++++++++++++++++++ yarn.lock | 16 ++--- 5 files changed, 93 insertions(+), 22 deletions(-) diff --git a/core/server/lib/mobiledoc.js b/core/server/lib/mobiledoc.js index cc0574db22..da7d2c8d5c 100644 --- a/core/server/lib/mobiledoc.js +++ b/core/server/lib/mobiledoc.js @@ -1,9 +1,9 @@ const path = require('path'); const errors = require('@tryghost/errors'); -const imageTransform = require('@tryghost/image-transform'); const logging = require('../../shared/logging'); const config = require('../../shared/config'); const storage = require('../adapters/storage'); +const imageTransform = require('@tryghost/image-transform'); let cardFactory; let cards; @@ -32,7 +32,14 @@ module.exports = { cardFactory = new CardFactory({ siteUrl: config.get('url'), contentImageSizes: config.get('imageOptimization:contentImageSizes'), - srcsets: config.get('imageOptimization:srcsets') + srcsets: config.get('imageOptimization:srcsets'), + canTransformImage(storagePath) { + const {ext} = path.parse(storagePath); + + return imageTransform.canTransformFiles() + && imageTransform.canTransformFileExtension(ext) + && typeof storage.getStorage().saveRaw === 'function'; + } }); cards = defaultCards.map((card) => { @@ -100,13 +107,7 @@ module.exports = { return await imageSize.getImageSizeFromUrl(parsedUrl.href); } - // TODO: extract conditional logic lifted from handle-image-sizes.js async function getLocalSize(url) { - // skip local images if adapter doesn't support size transforms - if (typeof storageInstance.saveRaw !== 'function') { - return; - } - // local storage adapter's .exists() expects image paths without any prefixes const subdirRegex = new RegExp(`^${urlUtils.getSubdir()}`); const contentRegex = new RegExp(`^/${urlUtils.STATIC_IMAGE_URL_PREFIX}`); @@ -115,10 +116,7 @@ module.exports = { const {dir, name, ext} = path.parse(storagePath); const [imageNameMatched, imageName, imageNumber] = name.match(/^(.+?)(-\d+)?$/) || [null]; - if (!imageNameMatched - || !imageTransform.canTransformFileExtension(ext) - || !(await storageInstance.exists(storagePath)) - ) { + if (!imageNameMatched || !(await storageInstance.exists(storagePath))) { return; } diff --git a/core/server/web/site/middleware/handle-image-sizes.js b/core/server/web/site/middleware/handle-image-sizes.js index 9027edfb90..a0228e3990 100644 --- a/core/server/web/site/middleware/handle-image-sizes.js +++ b/core/server/web/site/middleware/handle-image-sizes.js @@ -73,6 +73,11 @@ module.exports = function (req, res, next) { return; } + // exit early if sharp isn't installed to avoid extra file reads + if (!imageTransform.canTransformFiles()) { + return redirectToOriginal(); + } + const imagePath = path.relative(sizeImageDir, req.url); const {dir, name, ext} = path.parse(imagePath); const [imageNameMatched, imageName, imageNumber] = name.match(/^(.+?)(-\d+)?$/) || [null]; diff --git a/package.json b/package.json index bf13b2b390..869f390a62 100644 --- a/package.json +++ b/package.json @@ -45,10 +45,10 @@ "@tryghost/adapter-manager": "0.1.6", "@tryghost/errors": "0.2.0", "@tryghost/helpers": "1.1.27", - "@tryghost/image-transform": "0.2.4", + "@tryghost/image-transform": "1.0.0", "@tryghost/kg-card-factory": "2.1.1", "@tryghost/kg-default-atoms": "2.0.1", - "@tryghost/kg-default-cards": "2.3.2", + "@tryghost/kg-default-cards": "2.4.0", "@tryghost/kg-markdown-html-renderer": "2.0.1", "@tryghost/kg-mobiledoc-html-renderer": "3.0.1", "@tryghost/magic-link": "0.4.9", diff --git a/test/unit/lib/mobiledoc_spec.js b/test/unit/lib/mobiledoc_spec.js index 49c23bcef6..d6b2225760 100644 --- a/test/unit/lib/mobiledoc_spec.js +++ b/test/unit/lib/mobiledoc_spec.js @@ -6,6 +6,7 @@ const configUtils = require('../../utils/configUtils'); const mobiledocLib = require('../../../core/server/lib/mobiledoc'); const storage = require('../../../core/server/adapters/storage'); const urlUtils = require('../../../core/shared/url-utils'); +const mockUtils = require('../../utils/mocks'); describe('lib/mobiledoc', function () { afterEach(function () { @@ -14,6 +15,7 @@ describe('lib/mobiledoc', function () { configUtils.restore(); // ensure config changes are reset and picked up by next test mobiledocLib.reload(); + mockUtils.modules.unmockNonExistentModule(/sharp/); }); describe('mobiledocHtmlRenderer', function () { @@ -112,6 +114,72 @@ describe('lib/mobiledoc', function () { mobiledocLib.mobiledocHtmlRenderer.render(mobiledoc) .should.eql('
Birdies
'); }); + + it('does not render srcsets for non-resizable images', function () { + let mobiledoc = { + version: '0.3.1', + atoms: [], + cards: [ + ['image', { + cardWidth: '', + src: '/content/images/2020/07/animated.gif', + width: 4000, + height: 2000 + }] + ], + markups: [], + sections: [[10, 0]] + }; + + mobiledocLib.mobiledocHtmlRenderer.render(mobiledoc) + .should.eql('
'); + }); + + it('does not render srcsets when sharp is not available', function () { + mockUtils.modules.mockNonExistentModule('sharp', new Error(), true); + + let mobiledoc = { + version: '0.3.1', + atoms: [], + cards: [ + ['image', { + src: '/content/images/2018/04/NatGeo06.jpg', + width: 4000, + height: 2000 + }] + ], + markups: [], + sections: [ + [10, 0] + ] + }; + + mobiledocLib.mobiledocHtmlRenderer.render(mobiledoc) + .should.eql('
'); + }); + + it('does not render srcsets with incompatible storage engine', function () { + sinon.stub(storage.getStorage(), 'saveRaw').value(null); + + let mobiledoc = { + version: '0.3.1', + atoms: [], + cards: [ + ['image', { + src: '/content/images/2018/04/NatGeo06.jpg', + width: 4000, + height: 2000 + }] + ], + markups: [], + sections: [ + [10, 0] + ] + }; + + mobiledocLib.mobiledocHtmlRenderer.render(mobiledoc) + .should.eql('
'); + }); }); describe('populateImageSizes', function () { diff --git a/yarn.lock b/yarn.lock index 83cc7d079a..f2078ed76f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -417,10 +417,10 @@ "@tryghost/mobiledoc-kit" "^0.12.4-ghost.1" jsdom "16.2.2" -"@tryghost/image-transform@0.2.4": - version "0.2.4" - resolved "https://registry.yarnpkg.com/@tryghost/image-transform/-/image-transform-0.2.4.tgz#bc0e791c909beff03f85ce61f7f8c7ece1213c9e" - integrity sha512-4FXm8VnRsv9SbXFqRKhrc4U56h1Bx58UTRxAMsp0FA2Sq68AU9YmTtjFS5t6h+mA0eQ/I6O5BiS1qPMdoWg/BA== +"@tryghost/image-transform@1.0.0": + version "1.0.0" + resolved "https://registry.yarnpkg.com/@tryghost/image-transform/-/image-transform-1.0.0.tgz#0389adae9eae0feb879d884658150eec9b067038" + integrity sha512-4bIS8BcokyvjmOk2QGIzOToP+nhNRV98RtcMwajnL1O53Ko1R5fIw93U+6OyOgzDHpYy+NZDK37hkEamNkq5sQ== dependencies: "@tryghost/errors" "^0.2.0" bluebird "3.7.2" @@ -443,10 +443,10 @@ resolved "https://registry.yarnpkg.com/@tryghost/kg-default-atoms/-/kg-default-atoms-2.0.1.tgz#3223ef5210d73af02c53795e5a5f1a9c5fc5bd92" integrity sha512-0/Fx98ZIj/gyPglKg9HQP+cKPSBpbue1pnzh3E8hR4WXzqnSWMFA8VTUyMeI+8oNwxkxhZrWt5KifngHbBfw2A== -"@tryghost/kg-default-cards@2.3.2": - version "2.3.2" - resolved "https://registry.yarnpkg.com/@tryghost/kg-default-cards/-/kg-default-cards-2.3.2.tgz#247c9b472742467d4d624f5070f71c36ecacb07a" - integrity sha512-gYROUJJMoUUFPjY56YWHn9I/akKwEg51HrBkeyHaXC4xEyAuDwpMdRtToVObSJCoAPjs9nB+TifHYZGk32aeWg== +"@tryghost/kg-default-cards@2.4.0": + version "2.4.0" + resolved "https://registry.yarnpkg.com/@tryghost/kg-default-cards/-/kg-default-cards-2.4.0.tgz#2f8d8f4bd362a22cd13bf15af0d4689ac4568222" + integrity sha512-kl5Oa+UTtnruZe/6/45QRp5A9jwtxrhjVnop46S6Ojl1w2SVwWDgTkKV6V4nHhCW/I7MJcWbzaABFwrULlRBiA== dependencies: "@tryghost/kg-markdown-html-renderer" "^2.0.1" "@tryghost/url-utils" "^0.6.14"