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

upload the sample 2 (using force-cache inspired by only-if-cached #28

Conversation

anton-karlovskiy
Copy link
Contributor

@anton-karlovskiy anton-karlovskiy commented Aug 14, 2019

The story comes from developing such a sample as a purely client-side (no Service Worker approach) that tried to make conditional requests with only-if-cached set (https://fetch.spec.whatwg.org/#concept-request-cache-mode), which would only work for same-origin requests.

In the general scope, I think this approach is for trying request.cache(https://developer.mozilla.org/en-US/docs/Web/API/Request/cache), exactly "only-if-cached" mode.

Based on some investigation and research work, I thought only-if-cached mode is not most appropriate, force-cache mode is more suitable.
So currently I implemented force-cache mode.

In the process of developing this project, I faced some tricky issues like intercepting http requests.
I found the solution to applying "only-if-cached" in modifying the resource fetching request, specifically cache mode name.
I tried several articles and packages for http request interception. But they are only valid in the case of direct fetch request like AJAX call in the component. Our case is some sort of behind scene fetch request like ,

So for the purpose of proof of concept, I took the approach like fetching blob from resource URL and converting it to base64 encoded string and feed it to image src props.
In video tag case, also this kind of feeding seems possible. For now, I just focused on images.
But for dynamic component loading, I have not found the solution apart from using fetch event listener in the service worker.

https://network-aware-force-cache.firebaseapp.com/
It's deployed here on firebase.

I think the base64 encoding image approach is not a generic approach that developers can adopt in any use cases.
For now just implemented that way.

@jeffposnick
Copy link

My feedback on @addyosmani's original design was that it would be good to iterate over the set of URLs for different image qualities that might be present in the HTTP cache, try an only-if-cached request on each, and if there's a match on any, use that response and avoid making an HTTP request at all.

So, if you visit the page once when you're on a 4G connection, and then again later when you're on a 3G connection, the page should use the 4G version of the image is already in the HTTP cache instead of making a network request for the 3G version.

I don't think the code in this PR accomplishes that behavior. Here's some untested code that might be a starting point:

async function requestImage(baseURL, currentQuality) {
  const qualities = ['max-res', 'medium-res', 'low-res'];

  let response;

  for (const quality of qualities) {
    const url = baseURL.replace(/QUALITY_PLACEHOLDER/, quality);
    const response = await fetch(url, {
      cache: 'only-if-cached',
      // only-if-cached will only work for same-origin requests.
      mode: 'same-origin',
    });
    if (response.ok) {
      // Do something with the response body, and return JSX.
      return (...);
    }
    // If there's a cache miss, response.ok will be false.
  }

  // If we get this far, there's no match in the HTTP cache.
  // Make a network request using currentQuality:
  const url = baseURL.replace(/QUALITY_PLACEHOLDER/, currentQuality);
  const response = await fetch(url);
  // Do something with the response body, and return JSX.
  return (...);
}

// Use this like:
const jsx = await requestImage('/my-image-QUALITY_PLACEHOLDER.jpg', effectiveConnectionType);

@anton-karlovskiy
Copy link
Contributor Author

anton-karlovskiy commented Aug 21, 2019

Yes I agree. "I don't think the code in this PR accomplishes that behavior."
Thank you @jeffposnick for your code snippets.

@anton-karlovskiy
Copy link
Contributor Author

anton-karlovskiy commented Aug 21, 2019

Do you think only-if-cached is more appropriate than force-cache?
To be honest, I thought force-cache makes more sense based on the docs.

In the process of checking your codesnippet I got it that only-cache-if is for us. :) because we manually request if not cached.

@anton-karlovskiy
Copy link
Contributor Author

anton-karlovskiy commented Aug 22, 2019

FYI: updated the source according to your suggestion.

return response.blob();
});
console.log('only-if-cached feeding url => ', url);
if (imageBlob) break;

Choose a reason for hiding this comment

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

Based on the docs for only-if-cached, I believe that a cache miss will result in a valid Response object, but one with a status of 504. So I think this will end up breaking even if there is a cache miss.

That's why I suggested using response.ok (which will be false if the status is 504, but true if the status is 200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. @jeffposnick.

According to what I've tested, if a cache miss happens, the flow gets into catch block (try catch).
I think using response.ok makes more sense. I'll update the logic that way.

@anton-karlovskiy
Copy link
Contributor Author

anton-karlovskiy commented Aug 22, 2019

Thank you @jeffposnick.
Now we are able to show any media regardless of content-type. Originally I thought that there might be a limit to content type i.e. I thought we would use just one content type whether image or video because I had not found the solution to feeding resource to image or video JSX.
Thanks to this docs we can:

const objectURL = URL.createObjectURL(blob);
image.src = objectURL;

Please review that implementation.
Deployed here

Copy link

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

Thanks! I think that's generally what I had in mind, though I don't have a chance to test your solution thoroughly in different network situations.

@anton-karlovskiy
Copy link
Contributor Author

anton-karlovskiy commented Aug 23, 2019

Thanks! I think that's generally what I had in mind, though I don't have a chance to test your solution thoroughly in different network situations.

Thank you for teaching.
To be honest I resolve kind of serious mistake in this project with the help of your suggestion and code snippets.
I learned a lot through your professional approach.

I've tested in different network situations and confirmed.
But I hope you test this thoroughly when you can.
I also added console logs to make sure it's working as expected.

@jeffposnick
Copy link

Happy to have helped!

Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM

@addyosmani addyosmani merged commit 4722b63 into GoogleChromeLabs:master Aug 27, 2019
@anton-karlovskiy anton-karlovskiy deleted the feature/sample2(force-cache) branch August 29, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants