feat: Simplify image middleware with express-sharp update (#1830)

* feat: Simplify image middleware with express-sharp update The express-sharp library has updated to v3.0.0, which includes a [breaking change](magento-research/express-sharp#5) to the URL scheme, and therefore the public API of the library. This helps us simplify the code in addImgOptMiddleware, because we don't have to rewrite the URL as aggressively--it's closer to the Fastly paradigm. Closes #1808, and some other issues. test: coverage for addImgOptMiddleware changes * fix: WebP content negotiation. Closes #1842 * fixup prettier
magento · Oct 10, 2019 · 11f6888 · 11f6888
1 parent e93270f
commit 11f6888
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 96 deletions.
diff --git a/packages/pwa-buildpack/lib/Utilities/__tests__/addImgOptMiddleware.spec.js b/packages/pwa-buildpack/lib/Utilities/__tests__/addImgOptMiddleware.spec.js
@@ -1,6 +1,7 @@
 jest.mock('redis', () => ({
     redisClient: jest.fn(() => 'redis client object')
 }));
+const { parse } = require('querystring');
 const addImgOptMiddleware = require('../addImgOptMiddleware');
 const expressSharp = require('@magento/express-sharp');
 const apicache = require('apicache');
@@ -13,10 +14,19 @@ let app, config, filterMiddleware, req, res;
 
 const next = () => {};
 
-const rewritten = '/resize/100?url=%2Fproduct.jpg&progressive=true&format=webp';
+const testUrl = (url, method = 'GET') => {
+    addImgOptMiddleware(app, config);
+    const { pathname, search } = new URL(url, 'http://localhost');
+    req = {
+        method,
+        path: pathname,
+        query: parse(search.replace(/^\?/, '')),
+        url
+    };
+    filterMiddleware(req, res, next);
+};
 
 beforeEach(() => {
-    filterMiddleware = undefined;
     app = {
         use: jest.fn((_, middleware) => {
             filterMiddleware = middleware;
@@ -27,16 +37,10 @@ beforeEach(() => {
         cacheExpires: '1 day',
         cacheDebug: false
     };
-    req = {
-        url: '/product.jpg?width=100&format=pjpg&auto=webp',
-        query: {
-            width: 100,
-            format: 'pjpg',
-            auto: 'webp'
-        }
-    };
     res = {
-        set: jest.fn()
+        set: jest.fn(() => res),
+        status: jest.fn(() => res),
+        send: jest.fn(() => res)
     };
 });
 
@@ -78,45 +82,72 @@ test('cache uses redis if supplied', () => {
     expect(redis.redisClient).toHaveBeenCalledWith('redis client address');
 });
 
-test('rewrites requests with resize params to the express-sharp pattern', () => {
-    addImgOptMiddleware(app, config);
-    filterMiddleware(req, res, next);
-    expect(req.url).toBe(rewritten);
-    expect(mockSharpMiddleware).toHaveBeenCalledWith(req, res, next);
+test('translates plain jpeg params', () => {
+    testUrl('/prog-jpeg.jpg?width=500&format=jpg');
+    expect(req.query).toMatchObject({
+        width: '500',
+        format: 'jpeg'
+    });
 });
 
-test('adds height and crop if height is present', () => {
-    addImgOptMiddleware(app, config);
-    req.url = '/product.jpg?width=200&height=400';
-    req.query = {
-        width: 200,
-        height: 400
-    };
-    filterMiddleware(req, res, next);
-    expect(req.url).toBe('/resize/200/400?url=%2Fproduct.jpg&crop=true');
+test('translates progressive jpeg params', () => {
+    testUrl('/prog-jpeg.jpg?width=500&format=pjpg');
+    expect(req.query).toMatchObject({
+        width: '500',
+        format: 'jpeg',
+        progressive: true
+    });
+});
+
+test('adds height and crop if width and height is present', () => {
+    testUrl('/product.jpg?width=200&height=400');
+    expect(req.query).toMatchObject({
+        width: '200',
+        height: '400',
+        crop: true
+    });
 });
 
 test('translates query parameters if present', () => {
-    addImgOptMiddleware(app, config);
-    req.url = '/product.jpg?width=200&otherParam=foo';
-    req.query = {
-        width: 200,
-        otherParam: 'foo'
-    };
-    filterMiddleware(req, res, next);
-    expect(req.url).toBe('/resize/200?otherParam=foo&url=%2Fproduct.jpg');
+    testUrl('/product.jpg?width=200&auto=webp&otherParam=foo');
+    expect(req.query).toMatchObject({
+        otherParam: 'foo',
+        width: '200'
+    });
 });
 
-test('does nothing to URLs without resize parameters', () => {
-    addImgOptMiddleware(app, config);
-    const noResizeUrl = '/product.jpg?otherParam=foo';
-    req.url = noResizeUrl;
-    req.query = {
-        otherParam: 'foo'
-    };
-    filterMiddleware(req, res, next);
-    expect(req.url).toBe(noResizeUrl);
+test('does nothing to non-GET URLs', () => {
+    testUrl('/product.jpg?width=300&format=pjpg', 'POST');
+    expect(mockSharpMiddleware).not.toHaveBeenCalled();
+    expect(req.query).not.toMatchObject({
+        progressive: true
+    });
+});
+
+test('does nothing to non-image URLs', () => {
+    testUrl('/not-an-image.txt?width=300&format=pjpg');
     expect(mockSharpMiddleware).not.toHaveBeenCalled();
+    expect(req.query).not.toMatchObject({
+        progressive: true
+    });
+});
+
+test('does nothing to urls lacking any resize parameters', () => {
+    testUrl('/no-need-to-resize.png');
+    expect(mockSharpMiddleware).not.toHaveBeenCalled();
+    expect(req.query).not.toMatchObject({
+        progressive: true
+    });
+});
+
+test('sends a 500 when resize fails', () => {
+    mockSharpMiddleware.mockImplementationOnce(() => {
+        throw new Error(
+            "this is fine, i'm ok with the events that are unfolding currently"
+        );
+    });
+    testUrl('/product.jpg?auto=webp');
+    expect(res.status).toHaveBeenCalledWith(500);
 });
 
 test('recovers from missing apicache dep', () => {

diff --git a/packages/pwa-buildpack/lib/Utilities/addImgOptMiddleware.js b/packages/pwa-buildpack/lib/Utilities/addImgOptMiddleware.js
@@ -1,5 +1,4 @@
 const debug = require('../util/debug').makeFileLogger(__filename);
-const { URL, URLSearchParams } = require('url');
 let cache;
 let expressSharp;
 let missingDeps = '';
@@ -22,7 +21,12 @@ try {
  * UPWARD, and then used in various places: here, the UPWARD resolution
  * path itself, the `makeURL` function in the client, etc.
  */
-const wantsResizing = req => !!req.query.width;
+const imageExtensions = /\.(jpg|png|gif|webp)$/;
+const imageParameters = ['auto', 'format', 'width', 'height'];
+const wantsResizing = req =>
+    req.method === 'GET' &&
+    imageExtensions.test(req.path) &&
+    imageParameters.some(param => param in req.query);
 
 function addImgOptMiddleware(app, config) {
     const { backendUrl, cacheExpires, cacheDebug, redisClient } = config;
@@ -59,30 +63,20 @@ If possible, install additional tools to build NodeJS native dependencies:
 https://github.com/nodejs/node-gyp#installation`
         );
     } else {
-        const toExpressSharpUrl = (incomingUrl, incomingQuery) => {
-            const imageUrl = new URL(incomingUrl, backendUrl);
-            debug('imageUrl', imageUrl);
-
-            const optParamNames = ['auto', 'format', 'width', 'height'];
-
-            // Start with the original search params, so
-            // we can preserve any non-imageopt parameters
-            // others might want.
-            const params = new URLSearchParams(imageUrl.search);
-            for (const param of optParamNames) {
-                params.delete(param);
-            }
-            params.set('url', imageUrl.pathname);
-            if (incomingQuery.format === 'pjpg') {
-                params.set('progressive', 'true');
+        const toExpressSharpQuery = query => {
+            // Venia's makeUrl expects the Fastly style of imageopto params.
+            // https://docs.fastly.com/api/imageopto/
+            // Rewrite them just a little bit.
+            switch (query.format) {
+                case 'pjpg':
+                    query.progressive = true;
+                    query.format = 'jpeg';
+                    break;
+                case 'jpg':
+                    query.format = 'jpeg';
+                default:
+                    break;
             }
-            if (incomingQuery.auto === 'webp') {
-                params.set('format', 'webp');
-            }
-
-            const { width, height } = incomingQuery;
-            let rewrittenUrl = `https://0.0.0.0/resize/${width}`;
-
             // If we received height and width we should force crop since our
             // implementation of express sharp defaults fit to "outside" if crop
             // is falsy. `outside` sizes the image, retaining the aspect ratio
@@ -91,35 +85,28 @@ https://github.com/nodejs/node-gyp#installation`
             // height and width.
             //   https://github.com/magento-research/express-sharp/blob/develop/lib/transform.js#L23
             //   https://sharp.pixelplumbing.com/en/stable/api-resize/
-            if (height) {
-                rewrittenUrl += `/${height}`;
-                params.set('crop', true);
+            if (query.width && query.height) {
+                query.crop = true;
             }
 
-            const rewritten = new URL(rewrittenUrl);
-            rewritten.search = params.toString();
-
-            debug({ rewritten });
-
-            // make relative url
-            const url = rewritten.href.slice(rewritten.origin.length);
-
-            // make new query object for express-sharp to use
-            const query = {};
-            for (const [key, value] of params) {
-                query[key] = value;
-            }
-            debug({ url, query });
-            return { url, query };
+            debug('rewrote query to %o', query);
         };
+
         const filterAndRewriteSharp = (req, res, next) => {
             if (wantsResizing(req)) {
-                const { url, query } = toExpressSharpUrl(req.url, req.query);
-                req.url = url;
-                req.query = query;
-                res.set('X-Image-Compressed-By', 'PWA Studio Staging Server');
-                sharpMiddleware(req, res, next);
+                try {
+                    toExpressSharpQuery(req.query);
+                    res.set(
+                        'X-Image-Compressed-By',
+                        'PWA Studio Staging Server'
+                    );
+                    debug(`delegate ${req.url.toString()} to sharpMiddleware`);
+                    sharpMiddleware(req, res, next);
+                } catch (e) {
+                    res.status(500).send(e);
+                }
             } else {
+                debug(`${req.url.toString()} does not appear to want resizing`);
                 next();
             }
         };

diff --git a/packages/pwa-buildpack/package.json b/packages/pwa-buildpack/package.json
@@ -27,7 +27,7 @@
   "homepage": "https://github.com/magento/pwa-studio/tree/master/packages/pwa-buildpack#readme",
   "dependencies": {
     "@magento/directive-parser": "~0.1.7",
-    "@magento/express-sharp": "~2.1.2",
+    "@magento/express-sharp": "~3.0.0",
     "@magento/upward-js": "~3.0.0",
     "apicache": "~1.4.0",
     "boxen": "~3.0.0",

diff --git a/packages/venia-ui/upward.yml b/packages/venia-ui/upward.yml
@@ -12,7 +12,7 @@ veniaResponse:
         # handled by the top-level 'veniaProxy' object, which is a ProxyResolver
         # that passes the request through to the backing Magento server.
         - matches: request.url.pathname
-          pattern: '^/(graphql|rest)(/|$)'
+          pattern: '^/(graphql|rest|media)(/|$)'
           use: veniaProxy
         - matches: request.url.pathname
           pattern: '^/(robots\.txt|favicon\.ico|manifest\.json)'

diff --git a/yarn.lock b/yarn.lock
@@ -2116,10 +2116,10 @@
   resolved "https://registry.yarnpkg.com/@magento/eslint-config/-/eslint-config-1.4.1.tgz#8c4ea4d91e3f314ee21dee8f113c5588fdf595df"
   integrity sha512-HKo5qt4Y13SFzTpTHp3mHQQgQ8G/WH60b4hgn3DweWsCL9I+CMjpN5io74l8rYvqj/87BQ94nhQif69HQ+s0sw==
 
-"@magento/express-sharp@~2.1.2":
-  version "2.1.2"
-  resolved "https://registry.yarnpkg.com/@magento/express-sharp/-/express-sharp-2.1.2.tgz#68b25d262b6435095d4d01190ed95b8b7726a95f"
-  integrity sha512-JOZOnWYUkF7Q1xWKXH6LRkjrhKnotA4bXbYzORzaX9oKwmQy/vxvsk4k80kpGwcdgc7bsAGfdtxYr7VwPaZ8UQ==
+"@magento/express-sharp@~3.0.0":
+  version "3.0.0"
+  resolved "https://registry.yarnpkg.com/@magento/express-sharp/-/express-sharp-3.0.0.tgz#aca78bc0abeb52ed121dd28979981b5f397841f3"
+  integrity sha512-uf53xJVK0ZEOCeBMxVaIkfki2Nb4XgmCmyvgrBUvsSKUfjmppgxZw1blVLaJ/L9cKDb0OXM7fcyzSHonhf6VKw==
   dependencies:
     cors "^2.8.4"
     debug "^3.1.0"