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

Revamp the runtime caching option conversion. #1160

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

jeffposnick
Copy link
Contributor

R: @philipwalton @addyosmani @gauntface

Fixes #1096, which also contains an explanation of what's changing and why.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7e21126 on runtime-caching-options-changes into ** on v3**.

}),
broadcastCacheUpdate: joi.object().keys({
channelName: joi.string().required(),
options: joi.object(),

Choose a reason for hiding this comment

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

I believe the broadcast cache update object needs a channel name.

Copy link
Contributor Author

@jeffposnick jeffposnick Jan 5, 2018

Choose a reason for hiding this comment

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

Yup—that requirement is enforced one line up, on line 46.

maxAgeSeconds: joi.number().min(1),
}).or('maxEntries', 'maxAgeSeconds'),
networkTimeoutSeconds: joi.number().min(1),
plugins: joi.array().items(joi.object()),

Choose a reason for hiding this comment

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

How does this work? I.e. how does code in the node space get into the service worker? My main consideration here is that if this works, why support one way in the build process and a different way in the service worker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code that converts whatever is configured in the node build tools into a string representation that could be inserted into a service worker file is

https://github.com/GoogleChrome/workbox/blob/v3/packages/workbox-build/src/lib/runtime-caching-converter.js

and the string is used in the template at

<% if (runtimeCaching) { runtimeCaching.forEach(runtimeCachingString => {%><%= runtimeCachingString %><% });} %>`;

(None of that has changed recently.)

The syntax used to generate the service worker code is not exactly the same as the syntax used when executing the service worker runtime. Service worker execution implies that we're inside of the ServiceWorkerGlobalScope with the various definitions in the workbox namespace available. When the build configuration is used, we're inside of the node runtime environment, and we maintain a mapping of the keys in the config that correspond to the resulting runtime code.

I think in this PR the mapping between configuration supplied to node and resulting Workbox v3 service worker runtime code gets much closer to being 1:1, as we removed some differences that were mainly around to match sw-precache's config.

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

In general this looks fine to me - comments are more just calling out concerns, but they aren't actionable.

@jeffposnick
Copy link
Contributor Author

Based on the approval, I'm going to merge. Matt, if you've got any larger concerns about the way the runtimeCaching config is translated into the final service worker runtime code, I'm happy to hash those out in a separate issue.

@jeffposnick jeffposnick merged commit aa77cb7 into v3 Jan 5, 2018
@jeffposnick jeffposnick deleted the runtime-caching-options-changes branch January 5, 2018 20:30
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