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

Strict SW catalog image checking. #2392

Merged
merged 4 commits into from
May 12, 2020

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented May 11, 2020

Description

@Jordaneisenburger has explained the issue at hand very well in the related issue.

It is a fairly simple fix. All we have to do is to compare the file names instead of checking for text includes.

I could not replicate the issue at hand because could not find URLs like mentioned in the issue. But, I have tested the SW cache handling and it works fine.

@Jordaneisenburger it would be great if you can validate the issue against this PR.

Related Issue

Closes #2332

Verification Stakeholders

@jimbo
@Jordaneisenburger

Verification Steps

  1. In Admin UI Create Category1 with Product image 5xb40.png.
  2. In Admin UI Create Category2 with Product image xb40.png
  3. Run Venia pointing to above Backend.
  4. Load Category1 and verify the image.
  5. Load category2 and verify the image
  6. Also verify product image loads correctly on Product page, Home page , mini cart, cart

(I was able to reproduce the issue so updated steps)

Checklist

None

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 11, 2020
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress May 11, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 11, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

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

Generated by 🚫 dangerJS against d3ec624

tjwiebell
tjwiebell previously approved these changes May 12, 2020
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Looks good, and I've confirmed on a local that it does resolve the bug. I assume a test will come later, but nice work 👍

@m2-community-project m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress May 12, 2020
@revanth0212
Copy link
Contributor Author

Looks good, and I've confirmed on a local that it does resolve the bug. I assume a test will come later, but nice work 👍

I forgot to push the test. Done 👍

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented May 12, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2392.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.71

…mageCacheHandler.spec.js

Co-authored-by: Tommy Wiebell <twiebell@adobe.com>
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Review in Progress in Pull Request Progress May 12, 2020
@dpatil-magento dpatil-magento merged commit 0bc884b into develop May 12, 2020
@m2-community-project m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress May 12, 2020
@dpatil-magento dpatil-magento deleted the revanth/sw.better-image-cache-handling branch May 12, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-concept version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

[bug]: Serviceworker is not strict enough in checking 'catalog' cache resources
5 participants