Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate optimized Image URLs using configuration from GraphQL #1267

Merged
merged 8 commits into from
Jun 5, 2019
7 changes: 0 additions & 7 deletions packages/venia-concept/.env.dist
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@ MAGENTO_BACKEND_URL="https://release-dev-231-npzdaky-zddsyhrdimyra.us-4.magentos
# you can change this variable. Example:
# MAGENTO_WEBSITE_CODE="foo"
#
# Change the URL paths where the PWA expects Magento to serve catalog media.
# By default these are `/media/catalog/product` and `/media/catalog/category`,
# but if your Magento instance does something different, they're configurable
# here.
# MAGENTO_BACKEND_MEDIA_PATH_PRODUCT="/media/catalog/product"
# MAGENTO_BACKEND_MEDIA_PATH_CATEGORY="/media/catalog/category"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we dropping support for these entirely? If so I think that means this has to be a major version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue this isn't a major, and is fully backwards compatible for developers that were previously using them. To resolve the docroot bug, you would prefix these values with /pub, and update the proxy pattern in venia-upward.yml to '^/(graphql|rest|pub/media)(/|$)'.

An upgrade makes them obsolete, but nothing breaks. I could see it going both ways, if upward config or environment variables are considered API (ie. you used them in your custom app code).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nothing breaks I'm okay with that then. I think the next version is already going to be a major so it may be moot anyway.

#
# These legacy environment variables from 1.x are no longer necessary:
# - `MAGENTO_BACKEND_PUBLIC_PATH`: Since PWA Studio no longer makes _themes_,
# it is no longer necessary to follow theme directory structure.
Expand Down
14 changes: 4 additions & 10 deletions packages/venia-concept/src/util/__tests__/makeUrl.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,12 @@ test('prepends media path for product images', () => {
expect(makeUrl(relativePath, { type: 'image-product' })).toBe(
`${productBase}${relativePath}?${defaultParams}`
);
expect(makeUrl(absoluteUrls[2], { type: 'image-product' })).toBe(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test changes all related to removed functionality. See https://github.com/magento-research/pwa-studio/pull/1267/files#r287155267

'https://example.com/media/catalog/product/baz.png?auto=webp&format=pjpg'
);
});

test('prepends media path for relative category images', () => {
expect(makeUrl(relativePath, { type: 'image-category' })).toBe(
`${categoryBase}${relativePath}?${defaultParams}`
);
expect(makeUrl(absoluteUrls[2], { type: 'image-category' })).toBe(
'https://example.com/media/catalog/category/baz.png?auto=webp&format=pjpg'
);
});

test("doesn't prepend media path if it's already included", () => {
Expand All @@ -52,12 +46,12 @@ test("doesn't prepend media path if it's already included", () => {
).toBeTruthy();
});

test('rewrites absolute url when width is provided', () => {
test('appends opt params to absolute url when width is provided', () => {
const width = 100;
const raw = absoluteUrls[2];

expect(makeUrl(raw, { type: 'image-product', width })).toBe(
`https://example.com${productBase}/baz.png?auto=webp&format=pjpg&width=100`
`https://example.com/baz.png?auto=webp&format=pjpg&width=100`
);
});

Expand All @@ -73,7 +67,7 @@ test('removes absolute origin if configured to', () => {
jest.resetModules();
const width = 100;
const htmlTag = document.querySelector('html');
htmlTag.setAttribute('data-backend', 'https://cdn.origin:8000/');
htmlTag.setAttribute('data-media-backend', 'https://cdn.origin:8000/');
htmlTag.setAttribute('data-image-optimizing-origin', 'onboard');
const makeUrlAbs = require('../makeUrl').default;
expect(
Expand All @@ -88,7 +82,7 @@ test('prepends absolute origin if configured to', () => {
jest.resetModules();
const width = 100;
const htmlTag = document.querySelector('html');
htmlTag.setAttribute('data-backend', 'https://cdn.origin:9000/');
htmlTag.setAttribute('data-media-backend', 'https://cdn.origin:9000/');
htmlTag.setAttribute('data-image-optimizing-origin', 'backend');
const makeUrlAbs = require('../makeUrl').default;
expect(
Expand Down
102 changes: 50 additions & 52 deletions packages/venia-concept/src/util/makeUrl.js
Original file line number Diff line number Diff line change
@@ -1,34 +1,36 @@
// If the root template supplies the backend URL at runtime, use it directly
const { imageOptimizingOrigin, backend } = document.querySelector(
'html'
).dataset;
const useBackendForImgs = backend && imageOptimizingOrigin === 'backend';
const {
imageOptimizingOrigin,
mediaBackend = 'https://backend.test/media/'
} = document.querySelector('html').dataset;
const useBackendForImgs = imageOptimizingOrigin === 'backend';

// Tests if a URL begins with `http:` or `https:` or `data:`
const absoluteUrl = /^(data|http|https)?:/i;

// Simple path joiner that guarantees one and only one slash between segments
const joinUrls = (base, url) =>
(base.endsWith('/') ? base.slice(0, -1) : base) +
'/' +
(url.startsWith('/') ? url.slice(1) : url);
tjwiebell marked this conversation as resolved.
Show resolved Hide resolved

const mediaBases = new Map()
.set(
'image-product',
process.env.MAGENTO_BACKEND_MEDIA_PATH_PRODUCT ||
'/media/catalog/product'
)
.set(
'image-category',
process.env.MAGENTO_BACKEND_MEDIA_PATH_CATEGORY ||
'/media/catalog/category'
);
.set('image-product', 'catalog/product/')
.set('image-category', 'catalog/category/');

/**
* Creates an "optimized" url for a provided absolute or relative url based on
* Creates an "optimized" url for a provided relative url based on
* requested media type and width. Any image URLs (whose type begins with
* "image-" will also be optimized.)
*
* If a `type` is provided the `path` will be joined with the associated media
* base.
* - `/media/catalog/product/path/to/img.jpg`
* - `catalog/product/path/to/img.jpg`
*
* If a `width` is provided, "resize parameters" are added to the URL for
* middlewares (either onboard or backend) to return using the desired width
* and original media url.
* - `/media/catalog/product/path/to/img.jpg?width=500&auto=webp&format=pjpg
* - `catalog/product/path/to/img.jpg?width=500&auto=webp&format=pjpg
*
* If only `path` is provided it is returned unaltered.
*
Expand All @@ -38,46 +40,42 @@ const mediaBases = new Map()
* @param {number} props.width - the desired resize width of the image
*/
const makeOptimizedUrl = (path, { type, width } = {}) => {
const { href, origin } = window.location;
let urlObject = new URL(path, href);
// Immediate return if there's no image optimization to attempt
if (!type || !type.startsWith('image-')) {
return path;
}
supernova-at marked this conversation as resolved.
Show resolved Hide resolved

if (type) {
if (mediaBases.has(type)) {
const mediaBase = mediaBases.get(type);
// prepend media base if it isn't already part of the pathname
if (!urlObject.pathname.includes(mediaBase)) {
urlObject.pathname = mediaBase + urlObject.pathname;
}
}
// add image optimization parameters and optionally change origin
if (type.startsWith('image-')) {
if (useBackendForImgs) {
urlObject = new URL(
urlObject.href.slice(urlObject.origin.length),
backend
);
} else if (path.startsWith(backend) && !useBackendForImgs) {
// Some API responses include absolute URLs to images.
// The backend won't optimize images, so do not use this
// absolute URL; instead, use a relative URL which has a chance
// of being passed through image optimization.
urlObject = new URL(path.slice(backend.length), origin);
}
const params = new URLSearchParams(urlObject.search);
params.set('auto', 'webp'); // Use the webp format if available
params.set('format', 'pjpg'); // Use progressive JPGs at least
if (width) {
// resize!
params.set('width', width);
}
urlObject.search = params.toString();
const { origin } = window.location;
const isAbsolute = absoluteUrl.test(path);
let baseURL = new URL(path, mediaBackend);

// If URL is relative and has a supported type, prepend media base onto path
if (!isAbsolute && mediaBases.has(type)) {
const mediaBase = mediaBases.get(type);
if (!baseURL.pathname.includes(mediaBase)) {
baseURL = new URL(joinUrls(mediaBase, path), mediaBackend);
}
}

if (urlObject.origin === origin) {
return urlObject.href.slice(origin.length);
if (baseURL.href.startsWith(mediaBackend) && !useBackendForImgs) {
baseURL = new URL(baseURL.href.slice(baseURL.origin.length), origin);
}
return urlObject.href;

// Append image optimization parameters
const params = new URLSearchParams(baseURL.search);
params.set('auto', 'webp'); // Use the webp format if available
params.set('format', 'pjpg'); // Use progressive JPGs at least
if (width) {
// resize!
params.set('width', width);
}
baseURL.search = params.toString();

if (baseURL.origin === origin) {
return baseURL.href.slice(baseURL.origin.length);
}

return baseURL.href;
};

export default makeOptimizedUrl;
2 changes: 1 addition & 1 deletion packages/venia-concept/templates/open-document.mst
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!doctype html>
<html lang="en" data-image-optimizing-origin="{{env.IMAGE_OPTIMIZING_ORIGIN}}" data-backend="{{env.MAGENTO_BACKEND_URL}}">
<html lang="en" data-image-optimizing-origin="{{env.IMAGE_OPTIMIZING_ORIGIN}}" data-media-backend="{{mediaBackendURL}}">
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
Expand Down
12 changes: 0 additions & 12 deletions packages/venia-concept/validate-environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,6 @@ function validateEnvironment(env) {
example: 'sw.js',
default: 'sw.js'
}),
MAGENTO_BACKEND_MEDIA_PATH_PRODUCT: str({
desc:
'URL path where the PWA expects Magento to serve product media.',
example: '/media/catalog/product',
default: '/media/catalog/product'
}),
MAGENTO_BACKEND_MEDIA_PATH_CATEGORY: str({
desc:
'URL path where the PWA expects Magento to serve category media.',
example: '/media/catalog/category',
default: '/media/catalog/category'
}),
MAGENTO_BUILDPACK_PROVIDE_SECURE_HOST: bool({
desc:
'On first run, create a secure, unique hostname and generate a trusted SSL certificate.',
Expand Down
12 changes: 11 additions & 1 deletion packages/venia-concept/venia-upward.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ response:
# handled by the top-level 'proxy' object, which is a ProxyResolver
# that passes the request through to the backing Magento server.
- matches: request.url.pathname
pattern: '^/(graphql|rest|media)(/|$)'
pattern: '^/(graphql|rest)(/|$)'
use: proxy
# Requests to create an account are handled by the top-level 'index'
# object, which returns a generic app shell
Expand Down Expand Up @@ -129,6 +129,7 @@ appShell:
bundles: assetManifest.bundles
urlResolver: urlResolver
env: env
mediaBackendURL: mediaBackendURL

# The resource object provides information about the page type and which
# template to use for a requested resource.
Expand Down Expand Up @@ -238,6 +239,15 @@ urlResolverResult:
inline:
urlKey: request.url.pathname

# Extract base media URL from ServiceResolver result
mediaBackendURL: mediaBackendResult.data.storeConfig.secure_base_media_url

# Retrieve base media URL with ServiceResolver query
mediaBackendResult:
url: magentoGQL
query:
inline: 'query { storeConfig { secure_base_media_url } }'

# The magentoGQL object is a UrlResolver that returns the URL for the
# GraphQL endpoint using the MAGENTO_BACKEND_URL environment variable.
magentoGQL:
Expand Down