Generate optimized Image URLs using configuration from GraphQL #1267

tjwiebell · 2019-05-23T22:16:40Z

Description

Made changes necessary to source media URL from GraphQL, and made it so it was backwards compatible with the image optimization middleware that would not have access to this configuration value. This should mean backing instances that aren't using /pub as their document root, not require zero additional steps to get Venia up and running. This additionally supports media URLs that are completely external of the Magento backing system, which can be more common with larger instances (eg. an Amazon S3 bucket, or a CDN).

Related Issue

Closes #567

Verification Steps

onboard image opt w/ `/pub` as document root

Use default Magento backend and spin up Venia
With DevTools open, navigate to home page
Inspect image requests and verify that they are served and have X-Image-Compressed-By: PWA Studio Staging Server header and that they are optimized
Navigate around rest of application and verify no broken images or broken links

onboard image opt w/ `/` as document root

Spin up local Magento backend using / and document root
Run all the same steps as above (should observe /pub in all image requests now)

backend image opt w/ `/pub` as document root

Use Fastly enabled backend (https://master-7rqtwti-xjmpcdjpe6kzm.us-4.magentosite.cloud/), IMAGE_OPTIMIZING_ORIGIN=backend, and spin up Venia
With DevTools open, navigate to home page
Inspect image requests and verify that they don't have X-Image-Compressed-By: PWA Studio Staging Server header and that they are optimized by Fastly instead (should be webp, some Fastly headers)
Navigate around rest of application and verify no broken images or broken links

How Have YOU Tested this?

Ran manual tests above and jest

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

major (e.g x.0.0 - a breaking change)
minor (e.g 0.x.0 - a backwards compatible addition)
patch (e.g 0.0.x - a bug fix)

I'm considering this a patch because it is backwards compatible, and developers previously using variables to fix bug will notice no change.

Checklist:

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

- Update tests to reflect functionality change with absolute URLs

vercel · 2019-05-23T22:16:47Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-feature-graphql-media-urls.magento-research1.now.sh

tjwiebell · 2019-05-23T22:18:51Z

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


    if (type) {
-        if (mediaBases.has(type)) {
+        // If URL is relative and has a supported type, prepend media base onto path
+        if (!isAbsolute && mediaBases.has(type)) {


I made the decision to remove functionality that tries to inject media bases into absolute URLs. Logically, I couldn't think of a use case where we receive an absolute URL that doesn't already have the media base in it. If I'm missing something, let me know, and I will re-implement.

Unfortunately I don't know enough to be able to answer this confidently.

Ping @zetlen @bbatsche for a sanity check here.

tjwiebell · 2019-05-23T22:22:14Z

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

@@ -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(


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

supernova-at

Can drop some conditionals 👍

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

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

supernova-at · 2019-05-30T15:31:32Z

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


    if (type) {
-        if (mediaBases.has(type)) {
+        // If URL is relative and has a supported type, prepend media base onto path
+        if (!isAbsolute && mediaBases.has(type)) {


Unfortunately I don't know enough to be able to answer this confidently.

Ping @zetlen @bbatsche for a sanity check here.

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

supernova-at · 2019-05-30T16:22:36Z

packages/venia-concept/.env.dist

-#   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"


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

Yes and yes!

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).

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.

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

supernova-at · 2019-06-04T16:36:37Z

Code looks good, running verification steps now

supernova-at · 2019-06-04T16:51:12Z

@tjwiebell and I are going to pair on the verification steps

- Generate optimized URLs using new GraphQL query result

24ac85c

- Update tests to reflect functionality change with absolute URLs

tjwiebell commented May 23, 2019

View reviewed changes

Remove obsolete media path variables

daa853c

vercel bot deployed to staging May 24, 2019 14:29 View deployment

Merge branch 'develop' into feature/graphql-media-urls

8bbf106

vercel bot deployed to staging May 28, 2019 19:08 View deployment

Merge branch 'develop' into feature/graphql-media-urls

6b2c410

vercel bot requested a deployment to staging May 30, 2019 15:12 Abandoned

supernova-at self-assigned this May 30, 2019

vercel bot deployed to staging May 30, 2019 15:13 View deployment

supernova-at requested changes May 30, 2019

View reviewed changes

Address feedback

a21e8da

vercel bot deployed to staging May 31, 2019 16:15 View deployment

tjwiebell requested a review from supernova-at May 31, 2019 16:16

Merge branch 'develop' into feature/graphql-media-urls

1d2bde8

vercel bot deployed to staging June 4, 2019 16:36 View deployment

supernova-at approved these changes Jun 4, 2019

View reviewed changes

Merge branch 'develop' into feature/graphql-media-urls

60012fa

vercel bot deployed to staging June 5, 2019 17:20 View deployment

Merge branch 'develop' into feature/graphql-media-urls

1642d47

vercel bot deployed to staging June 5, 2019 21:16 View deployment

dpatil-magento merged commit 03b5d45 into develop Jun 5, 2019

supernova-at deleted the feature/graphql-media-urls branch July 25, 2019 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate optimized Image URLs using configuration from GraphQL #1267

Generate optimized Image URLs using configuration from GraphQL #1267

tjwiebell commented May 23, 2019 •

edited by sirugh

Loading

vercel bot commented May 23, 2019

tjwiebell May 23, 2019

supernova-at May 30, 2019

tjwiebell May 23, 2019

supernova-at left a comment

supernova-at May 30, 2019

supernova-at May 30, 2019

zetlen May 30, 2019

tjwiebell May 30, 2019

supernova-at Jun 4, 2019

supernova-at commented Jun 4, 2019

supernova-at commented Jun 4, 2019

Generate optimized Image URLs using configuration from GraphQL #1267

Generate optimized Image URLs using configuration from GraphQL #1267

Conversation

tjwiebell commented May 23, 2019 • edited by sirugh Loading

Description

Related Issue

Verification Steps

onboard image opt w/ /pub as document root

onboard image opt w/ / as document root

backend image opt w/ /pub as document root

How Have YOU Tested this?

Screenshots / Screen Captures (if appropriate):

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at commented Jun 4, 2019

supernova-at commented Jun 4, 2019

tjwiebell commented May 23, 2019 •

edited by sirugh

Loading

onboard image opt w/ `/pub` as document root

onboard image opt w/ `/` as document root

backend image opt w/ `/pub` as document root