Fix makeUrl for Fastly (#1039)

* Fix makeUrl
* Remove media path logic from makeUrl
* Remove invalid flag
* Update packages/venia-concept/src/util/makeUrl.js jsdoc

packages/venia-concept/src/util/__tests__/makeUrl.spec.js

@@ -1,46 +1,83 @@
 import makeUrl from '../makeUrl';
 
-test('returns absolute path unmodified', () => {
-    const absoluteUrls = [
-        'data://example.com/foo.png',
-        'http://example.com/bar.png',
-        'https://example.com/baz.png'
-    ];
+const productBase = '/media/catalog/product';
+const categoryBase = '/media/catalog/category';
+const resizeBase = '/img/resize';
 
+const relativePath = '/some/path/to/img.jpg';
+const absoluteUrls = [
+    'data://example.com/foo.png',
+    'http://example.com/bar.png',
+    'https://example.com/baz.png'
+];
+
+test('returns absolute url unmodified when called with no options', () => {
     absoluteUrls.forEach(url => {
         expect(makeUrl(url)).toBe(url);
     });
 });
 
-test('returns untyped, unresized url unmodified', () => {
-    const relativeUrl = '/some/path/to/img.jpg';
-    expect(makeUrl(relativeUrl)).toBe(relativeUrl);
+test('returns relative path unmodified when called with no options', () => {
+    expect(makeUrl(relativePath)).toBe(relativePath);
+});
+
+test('throws if type is unrecognized', () => {
+    const invalidType = 'invalid';
+    const thrower = () => {
+        makeUrl(relativePath, { type: invalidType });
+    };
+
+    expect(thrower).toThrow(invalidType);
+});
+
+test('prepends media path for product images', () => {
+    expect(makeUrl(relativePath, { type: 'image-product' })).toBe(
+        `${productBase}${relativePath}`
+    );
+    expect(makeUrl(absoluteUrls[2], { type: 'image-product' })).toBe(
+        `${productBase}${new URL(absoluteUrls[2]).pathname}`
+    );
 });
 
-test('normalizes slash paths on typed urls', () => {
-    const leadingSlash = '/foo.jpg';
-    const noLeadingSlash = 'foo.jpg';
-    expect(makeUrl(leadingSlash, { type: 'image-product' })).toBe(
-        '/media/catalog/product/foo.jpg'
+test('prepends media path for category images', () => {
+    expect(makeUrl(relativePath, { type: 'image-category' })).toBe(
+        `${categoryBase}${relativePath}`
     );
-    expect(makeUrl(noLeadingSlash, { type: 'image-category' })).toBe(
-        '/media/catalog/category/foo.jpg'
+    expect(makeUrl(absoluteUrls[2], { type: 'image-category' })).toBe(
+        `${categoryBase}${new URL(absoluteUrls[2]).pathname}`
     );
 });
 
-test('errors on unrecognized type', () => {
-    expect(() => makeUrl('url', { type: 'invalid' })).toThrow(
-        'Unrecognized media type invalid'
+test("doesn't prepend media path if it's already included", () => {
+    const cachedPath = `${productBase}/foo.jpg`;
+
+    expect(makeUrl(cachedPath, { type: 'image-product' })).toBe(cachedPath);
+});
+
+test('rewrites absolute url when width is provided', () => {
+    const width = 100;
+    const raw = absoluteUrls[2];
+    const { pathname } = new URL(raw);
+
+    expect(makeUrl(raw, { width })).toBe(
+        `${resizeBase}/${width}?url=${encodeURIComponent(pathname)}`
     );
 });
 
-test('resizes urls to specific widths', () => {
-    expect(
-        makeUrl('some/category.jpg', { type: 'image-category', width: 160 })
-    ).toBe(
-        '/img/resize/160?url=%2Fmedia%2Fcatalog%2Fcategory%2Fsome%2Fcategory.jpg'
+test('rewrites relative path when width is provided', () => {
+    const width = 100;
+
+    expect(makeUrl(relativePath, { width })).toBe(
+        `${resizeBase}/${width}?url=${encodeURIComponent(relativePath)}`
     );
-    expect(makeUrl('/some/other/thing.jpg', { width: 480 })).toBe(
-        '/img/resize/480?url=%2Fsome%2Fother%2Fthing.jpg'
+});
+
+test('includes media path when rewriting for resizing', () => {
+    const width = 100;
+
+    expect(makeUrl(relativePath, { width, type: 'image-product' })).toBe(
+        `${resizeBase}/${width}?url=${encodeURIComponent(
+            productBase + relativePath
+        )}`
     );
 });

packages/venia-concept/src/util/makeUrl.js

@@ -1,64 +1,74 @@
-import memoize from 'lodash/memoize';
-
-// Tests if a URL begins with `http://` or `https://` or `data:`
-const absoluteUrl = /^(data|https|http)?:/i;
-
-// Simple path joiner that guarantees one and only one slash between segments
+// ensure there's exactly one slash between segments
 const joinUrls = (base, url) =>
     (base.endsWith('/') ? base.slice(0, -1) : base) +
     '/' +
     (url.startsWith('/') ? url.slice(1) : url);
 
-// TODO: make this even more dynamic, like an open registry
-const mediaPathPrefixes = {
-    'image-product':
+const mediaBases = new Map()
+    .set(
+        'image-product',
         process.env.MAGENTO_BACKEND_MEDIA_PATH_PRODUCT ||
-        '/media/catalog/product',
-    'image-category':
-        process.env.MAGENTO_BACKEND_MEDIA_PATH_CATALOG ||
-        '/media/catalog/category'
-};
-
-// Should produce `/img/resize` in the default case
-const resizeBase = joinUrls(process.env.IMAGE_SERVICE_PATH || '/img', 'resize');
-
-// `makeResizedUrl("/path/to/jpeg", { width: 80 })` returns
-// "/img/resize/80?url=%2Fpath%2fto%2fjpeg"
-function makeResizedUrl(url, width) {
-    return `${joinUrls(resizeBase, width.toString())}?url=${encodeURIComponent(
-        url
-    )}`;
-}
-
-// Should fail silently on a null value, so default to the empty string
-// so that string methods work
-function formatUrl(url = '', opts = {}) {
-    const { type, width } = opts;
-    if (absoluteUrl.test(url) || !(type || width)) {
-        // Absolute URLs shouldn't be resized
-        // Without a type or a width, we don't know how to transform this url
-        return url;
-    }
+            '/media/catalog/product'
+    )
+    .set(
+        'image-category',
+        process.env.MAGENTO_BACKEND_MEDIA_PATH_CATEGORY ||
+            '/media/catalog/category'
+    );
 
-    // we need to throw on an unrecognized `type`, so we may have to assign this
-    // in another code branch, hence the "let"
-    let formattedUrl = url;
+const resizeBase = joinUrls(
+    process.env.IMAGE_SERVICE_PATH || '/img/',
+    '/resize/'
+);
+/**
+ * Creates an "optimized" url for a provided absolute or relative url based on
+ * requested media type and width.
+ *
+ * If a `type` is provided the `path` will be joined with the associated media
+ * base.
+ *  - "/media/catalog/product/some/path/to/img.jpg"
+ *
+ * If a `width` is provided a "resize url" is returned using the desired width
+ * and original media url.
+ *  - /img/resize/640?url=%2Fmedia%2Fcatalog%2Fproduct%2Fsome%2Fpath%2Fto%2F/image.jpg
+ *
+ * If only `path` is provided it is returned unaltered.
+ *
+ * @param {string} path - absolute or relative url to resource.
+ * @param {Object} props - properties describing desired optimizations
+ * @param {string} props.type - "image-product" or "image-category"
+ * @param {number} props.width - the desired resize width of the image
+ */
+const makeOptimizedUrl = (path, { type, width } = {}) => {
+    const { location } = window;
+    const urlObject = new URL(path, location.href);
+    const params = new URLSearchParams(urlObject.search);
 
     if (type) {
-        if (!mediaPathPrefixes.hasOwnProperty(type)) {
+        if (!mediaBases.has(type)) {
             throw new Error(`Unrecognized media type ${type}`);
         }
-        // "/path/to/jpeg" becomes e.g. "/media/catalog/product/path/to/jpeg"
-        formattedUrl = joinUrls(mediaPathPrefixes[type], url);
+
+        const mediaBase = mediaBases.get(type);
+
+        // prepend media base if it isn't already part of the pathname
+        if (!urlObject.pathname.includes(mediaBase)) {
+            urlObject.pathname = joinUrls(mediaBase, urlObject.pathname);
+        }
+
+        // check for width before returning
     }
 
-    // if a width exists, make a resized URL!
-    return width ? makeResizedUrl(formattedUrl, width) : formattedUrl;
-}
+    if (width) {
+        // set pathname as query param
+        // encodeURIComponent would be redundant
+        params.set('url', urlObject.pathname);
 
-// Each combination of type, url, and width is computed only once and cached.
-// storeUrlKey defines the cache key as a function of the arguments.
-const storeUrlKey = (url, { width = '', type = '' } = {}) =>
-    `${type}%%${url}%%${width}`;
+        return `${resizeBase}${width}?${params}`;
+    }
+
+    // return unaltered path if we didn't operate on it
+    return type ? urlObject.pathname : path;
+};
 
-export default memoize(formatUrl, storeUrlKey);
+export default makeOptimizedUrl;

packages/venia-concept/venia-upward.yml

@@ -9,7 +9,7 @@ response:
       pattern: '^/img/resize/(\d+)'
       use: imageOpt
     - matches: request.url.pathname
-      pattern: '^/(graphql|rest|media/)'
+      pattern: '^/(graphql|rest|media|cache/)'
       use: proxy
     - matches: request.url.pathname
       pattern: '^/create-account'
@@ -31,9 +31,13 @@ response:
 # the image resizing middleware at `/img/` before the UpwardPlugin declares
 # middleware, so a request should never get here.
 
-# In production mode, this forwards to Fastly where it is present, and drops
-# back to the uncompressed image otherwise. If this will run on a non-FastlyIO
-# server, an onboard resizing service is highly recommended.
+# In production mode when USE_FASTLY is true, the Venia PWA has a
+# Fastly-specific URL factory. It should also never hit this route.
+
+# In production mode when the PWA has NOT built a Fastly-specific URL factory,
+# this forwards to Fastly where it is present, and drops back to the
+# uncompressed image otherwise. If this will run on a non-FastlyIO server, an
+# onboard resizing service is highly recommended.
 imageOpt:
   inline:
     status: 301