[PWA-1128] fix(buildpack): Stop apicache from overriding good cache headers.

[zetlen]

Description

Fixes a bug in addImageOptMiddleware where when attached, the middleware breaks all cache functionality but its own. The middleware was applying to every request, rather than every image request, and the apicache module was setting Cache-Control: no-cache no-store must-revalidate on every response it didn't recognize as a valid image.

• Added a route pattern to the app.use() call in addImageOptMiddleware, to narrow down what requests the apicache middleware handles to the intended set of only image requests.
• Updated hastily dependency to use a new version which exports its list of supported extensions, so that the image middleware can build its route pattern based on what the currently installed image optimizer supports.
• Adjusted addImageOptMiddleware unit test to be less brittle.

Related Issue

Closes PWA-1128.

Tracked as a client-reported escalation through the following tickets:
PWA-1128
MC-39132
MDVA-32861

Acceptance

Verification Stakeholders

@novykov
@davemacaulay
@dpatil-magento

Any other stakeholders from the mentioned Support tickets

Verification Steps

1. Run yarn && yarn run build && yarn run stage:venia.
2. Get the response headers of an image request by:
   a. Run this command at a shell prompt: t(){ curl -I "$1/VeniaLogo-n77.svg" }; t <staging_server_url> (replacing <staging_server_url> with your local staging server URL
   b. Or, open a browser window with dev tools open, load the staging server, and observe the cache control headers on server responses
3. Observe that this request no longer has no-cache headers.

Screenshots / Screen Captures (if appropriate)

Checklist

• I have added tests to cover my changes, if necessary.
• I have added translations for new strings, if necessary.
• I have updated the documentation accordingly, if necessary.

[PWAStudioBot]

	Messages
📖	Associated JIRA tickets: PWA-1128.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 2557ea6

[jimbo]

Thanks for investigating the issue and submitting this PR. Will get QA on it soon.

[dpatil-magento]

@zetlen Some observations, please take a look -

1. Some image requests still have x-cache MISS, not sure if this also needs to be fixed.
   

2. Seeing new cache control public
   

3. Webpagetest score for cache static content and effective use of CDN has reduced compared to develop.

https://www.webpagetest.org/performance_optimization.php?test=201124_Di5M_d8e4fa875cafd991ebe32cdc9a2c789e&run=1#cache_static_content

[zetlen]

@dpatil-magento Be careful that your tests are with an empty cache and ServiceWorkers disabled, BUT you don't have cache disabled. That's the only way you will get reliable headers.

I've added a couple of commits which fix the files you've called out. Let's run another test!

[dpatil-magento]

Thanks @zetlen , tested with both disabled/enabled service worker and cache.

After new commit > cache static content score looks great but CDN score went down, please check.

https://www.webpagetest.org/performance_optimization.php?test=201130_DiE4_7c5b8754e176cccde43ce708b68d4dbc&run=2#cache_static_content

[tjwiebell]

After new commit > cache static content score looks great but CDN score went down, please check.

@dpatil-magento
I wouldn't expect this score to change given these PR deployments are on AWS and Fastly wouldn't touch anything but image requests (since those are proxied to the backend). If we weren't currently dealing with that bug of MCloud not caching asset requests, I would only expect this score to be near perfect with UPWARD-PHP/Cloud deployments that have Fastly enabled.

[zetlen]

It looks like the web server isn't automatically gzipping responses. That should be the web tier's job, not the PWA app server's job.

[dpatil-magento]

Thanks @zetlen , tested with both disabled/enabled service worker and cache.

After new commit > cache static content score looks great but CDN score went down, please check.

https://www.webpagetest.org/performance_optimization.php?test=201130_DiE4_7c5b8754e176cccde43ce708b68d4dbc&run=2#cache_static_content

Confirmed over call with @zetlen , its existing bug just uncovered part of this PR changes. Will have seperate bug for this.

[dpatil-magento]

QA Approved.

[jimbo]

It looks like the web server isn't automatically gzipping responses. That should be the web tier's job, not the PWA app server's job.

To be clear, is this PR shifting cache-control headers from the web tier's job into the PWA app server's job?

[tjwiebell]

To be clear, is this PR shifting cache-control headers from the web tier's job into the PWA app server's job?

@jimbo - That is mostly correct, yes. In this PR all static and media requests served by upward-js now get their headers from the app; the only lingering static request that isn't covered is the app shell (index.html). The static rule is hard-coded, which isn't ideal, but we've discussed adding header support to the directory resolver, so cache rules are defined by upward config, making this easier to opt-out of/override. It is still worth pointing out that a web server or proxy-cache doesn't have to respect these values either, which is the problem we're currently facing in Cloud/UPWARD-PHP.

I'm also a bit confused by this decision, because we've pulled other things out of the application tier like compression (and are now adding it back), so it's unclear what our position is on this responsibility. My current assumption is that we want to enforce as many best practices in the application layer, but make them simple to opt-out of if you would prefer to handle that yourself (either overriding our application defaults or doing at a different tier). To put it a bit bluntly, I would maybe call this idiot proofing or making the app reliable by default.

@zetlen - Could you help align us on the shift this PR puts on our position in regards to this responsibility?

[fooman]

Mentally I think of upward as .htaccess for a pwa site and more generally I believe the app should have the ability to control how it gets cached - for example the server would have no chance of knowing if a gif is static for eternity or if it's an animated weather map that gets refreshed every 10 minutes. Having it provide cache-control headers would fit.

[jimbo]

Approving this. Let's check back after we fix Fastly for real, though.