Skip to content
This repository has been archived by the owner on Jan 23, 2021. It is now read-only.

added: ability to supply html string as dependency for dynamicUrlToDependencies #262

Merged
merged 1 commit into from Mar 8, 2017

Conversation

HenrikJoreteg
Copy link
Contributor

First off thanks @jeffposnick and co for your work on this awesome lib!

This PR addresses the use-case I mentioned here: #156 (comment)

To summarize, we have a scenario where we don't have a static "base" html file or even a template file that changes each time the cached "dynamicUrl" should be considered changed. Instead, our dependency is really the resulting HTML string that we generate for that environment. The / dynamic route that we're trying to precache is the result of an aggregated environment-specific config object and an HTML template. I could list out a set of config files that is used to aggregate that config, but doing so would be brittle and error prone. Instead, this suggested change enables passing the contents of the file itself as a dependency for the dynamicUrlToDependencies option.

So if the sw-precache library gets a string instead of an array it will use that to generate a hash, and add it to the total size.

This PR represents the change I've made locally to make this work for us. So far it's working well for us and I'd love to see this adopted upsteam. If this looks/sounds reasonable to you, I'll tack on some updates to the docs as well. I just wanted to float the idea first before I spend time on that.

Please let me know what you think when you have a sec. Thanks!

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@HenrikJoreteg
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@gauntface
Copy link

The changes seem sane to me. Not sure about the "cumulativeSize" change largely because I don't know how that value is used.

@HenrikJoreteg
Copy link
Contributor Author

HenrikJoreteg commented Mar 7, 2017

@gauntface cool! The cumulativeSize change seems to be used primarily to provide useful output when in verbose mode.

Judging by the comment in the snippet below and how it currently builds cumulativeSize when it gets an array of dependencies, it seems like it's just a loose estimate as CLI output to show roughly how big your total precache will be:

(snippet from here: https://github.com/GoogleChrome/sw-precache/blob/master/lib/sw-precache.js#L220-L221)

      filesAndSizesAndHashes.forEach(function(fileAndSizeAndHash) {
        // Let's assume that the response size of a server-generated page is roughly equal to the
        // total size of all its components.
        cumulativeSize += fileAndSizeAndHash.size;
        concatenatedHashes += fileAndSizeAndHash.hash;
      });

@HenrikJoreteg
Copy link
Contributor Author

HenrikJoreteg commented Mar 7, 2017

Also, @gauntface, i saw your tweet about sw-helpers which looks cool. The only thing is, as far as I can tell, using that would require switching mechanisms for all this stuff entirely and we've already got sw-precache baked in and working well. This tweak just fixes this one little snag we ran into. Ultimately I like the look of the sw-helpers stuff, though. But right now I just wanna get our app shipped.

@jeffposnick
Copy link
Contributor

Thanks for the submission, @HenrikJoreteg! This seems like a use case that's worthwhile and straightforward to support, and I'll give the PR a review in the near future.

@jeffposnick jeffposnick merged commit c6aa26f into GoogleChromeLabs:master Mar 8, 2017
@jeffposnick
Copy link
Contributor

I'll update the documentation, and cut a new minor release that includes this change (also in the near future).

@HenrikJoreteg
Copy link
Contributor Author

sweet, thanks @jeffposnick

@jeffposnick
Copy link
Contributor

@HenrikJoreteg, it's been published as 5.1.0.

@HenrikJoreteg
Copy link
Contributor Author

@jeffposnick 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants