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

Add a workbox-window package #1827

Merged
merged 5 commits into from
Jan 11, 2019
Merged

Add a workbox-window package #1827

merged 5 commits into from
Jan 11, 2019

Conversation

philipwalton
Copy link
Member

This PR is an initial implementation of the workbox-window proposal.

Most of the implementation is purely additive, but a few things have changed in the build process. Mainly, an additional build-window-packages gulp task has been added that builds workbox-window since its build format is ES modules rather than IIFEs and browser globals.

I've also added a new templates route, that allow me to render content dynamically via nunjucks if the route matches the pattern *.tmp.*. This is how I test dynamic SW updates for scripts with the same URL in my integration tests.

R: @jeffposnick

Copy link
Contributor

@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.

Mostly small changes not affecting the actual code. Thanks for getting this out there!

gulp-tasks/utils/build-window-bundle.js Outdated Show resolved Hide resolved
gulp-tasks/utils/build-window-bundle.js Outdated Show resolved Hide resolved
gulp-tasks/utils/build-window-bundle.js Outdated Show resolved Hide resolved
test/workbox-window/integration/test.js Outdated Show resolved Hide resolved
test/workbox-window/integration/test.js Outdated Show resolved Hide resolved
packages/workbox-window/Workbox.mjs Outdated Show resolved Hide resolved
packages/workbox-window/Workbox.mjs Outdated Show resolved Hide resolved
packages/workbox-window/Workbox.mjs Outdated Show resolved Hide resolved
packages/workbox-window/Workbox.mjs Outdated Show resolved Hide resolved
packages/workbox-window/Workbox.mjs Outdated Show resolved Hide resolved
@philipwalton
Copy link
Member Author

Ok, I think I've addressed all your comments. PTAL.

@workbox-pr-bot
Copy link
Collaborator

PR-Bot Size Plugin

Changed File Sizes

File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.51 KB 3.60 KB +3% 1.56 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.12 KB 1.88 KB +68% 939 B ☠️
packages/workbox-build/build/index.js 4.02 KB 3.64 KB -9% 1.36 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.88 KB 3.15 KB -19% 1.29 KB 🎉
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 587 B 594 B +1% 350 B
packages/workbox-cli/build/app.js 6.76 KB 5.58 KB -17% 1.98 KB 🎉
packages/workbox-cli/build/bin.js 2.32 KB 1.16 KB -50% 580 B 🎉
packages/workbox-core/build/workbox-core.prod.js 7.47 KB 6.21 KB -17% 2.80 KB 🎉
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js 660 B 667 B +1% 323 B
packages/workbox-precaching/build/workbox-precaching.prod.js 5.80 KB 5.27 KB -9% 2.21 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.63 KB 1.53 KB -6% 764 B
packages/workbox-routing/build/workbox-routing.prod.js 2.87 KB 3.28 KB +14% 1.43 KB ☠️
packages/workbox-strategies/build/workbox-strategies.prod.js 5.09 KB 4.76 KB -7% 1.19 KB
packages/workbox-streams/build/workbox-streams.prod.js 1.57 KB 1.39 KB -12% 683 B 🎉
packages/workbox-sw/build/workbox-sw.js 1.50 KB 1.51 KB +1% 818 B
packages/workbox-webpack-plugin/build/generate-sw.js 8.04 KB 5.29 KB -34% 1.84 KB 🎉
packages/workbox-webpack-plugin/build/index.js 742 B 349 B -53% 255 B 🎉
packages/workbox-webpack-plugin/build/inject-manifest.js 10.30 KB 6.91 KB -33% 2.40 KB 🎉

New Files

File Size GZipped
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js 1.74 KB 877 B

All File Sizes

View Table
File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.51 KB 3.60 KB +3% 1.56 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js 1.12 KB 1.88 KB +68% 939 B ☠️
packages/workbox-build/build/_types.js 41 B 41 B 0% 61 B
packages/workbox-build/build/index.js 4.02 KB 3.64 KB -9% 1.36 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js 3.88 KB 3.15 KB -19% 1.29 KB 🎉
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 587 B 594 B +1% 350 B
packages/workbox-cli/build/app.js 6.76 KB 5.58 KB -17% 1.98 KB 🎉
packages/workbox-cli/build/bin.js 2.32 KB 1.16 KB -50% 580 B 🎉
packages/workbox-core/build/workbox-core.prod.js 7.47 KB 6.21 KB -17% 2.80 KB 🎉
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js 1.74 KB 877 B
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js 660 B 667 B +1% 323 B
packages/workbox-precaching/build/workbox-precaching.prod.js 5.80 KB 5.27 KB -9% 2.21 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.63 KB 1.53 KB -6% 764 B
packages/workbox-routing/build/workbox-routing.prod.js 2.87 KB 3.28 KB +14% 1.43 KB ☠️
packages/workbox-strategies/build/workbox-strategies.prod.js 5.09 KB 4.76 KB -7% 1.19 KB
packages/workbox-streams/build/workbox-streams.prod.js 1.57 KB 1.39 KB -12% 683 B 🎉
packages/workbox-sw/build/workbox-sw.js 1.50 KB 1.51 KB +1% 818 B
packages/workbox-webpack-plugin/build/generate-sw.js 8.04 KB 5.29 KB -34% 1.84 KB 🎉
packages/workbox-webpack-plugin/build/index.js 742 B 349 B -53% 255 B 🎉
packages/workbox-webpack-plugin/build/inject-manifest.js 10.30 KB 6.91 KB -33% 2.40 KB 🎉

Workbox Aggregate Size Plugin

9.86KB gzip'ed (66% of limit)
24.19KB uncompressed

@jeffposnick jeffposnick merged commit 15c80c0 into next Jan 11, 2019
@madmoizo
Copy link

A controlled promise (@jeffposnick proposition from the past) would be a nice addition to avoid some boilerplate w3c/ServiceWorker#799 (comment)

@jadjoubran
Copy link
Collaborator

jadjoubran commented Jan 16, 2019

I noticed while testing that the workbox copyLibraries build/ wouldn't copy the workbox-window package. I personally don't need this, but let me know if you think other people would need it so I open an issue

@philipwalton
Copy link
Member Author

A controlled promise (@jeffposnick proposition from the past) would be a nice addition to avoid some boilerplate w3c/ServiceWorker#799 (comment)

@frlinw originally, my design doc did have a controlled promise (as well as an activated promise). But I removed it from the API due to the fact that it would only resolve on the page load that installed the SW for the very first time.

And that would lead to situations like this, where some code in a function would never run:

const wb = new Workbox('sw.js');
wb.register();

await wb.controlling;

// This message will not get sent on any pageload after the first one.
wb.messageSW({...});

Alternatively, the above problem could be solved if the controlled promise resolved on all page loads, but then you'd lose the ability to do something when the SW is first controlling the page.

So, I guess that main question is: do you have a use case for needing to await a SW controlling the page on all page loads? If so, can you elaborate?

@philipwalton
Copy link
Member Author

I noticed while testing that the workbox copyLibraries build/ wouldn't copy the workbox-window package...if you think other people would need it so I open an issue

@jadjoubran yes, please do!

@madmoizo
Copy link

madmoizo commented Jan 16, 2019

it would only resolve on the page load that installed the SW for the very first time.

@philipwalton what do you think about that ?

function controlled () {
  return new Promise((resolve) => {
    // resolve immediately if a service worker is already in control (every page loads after the first one)
    if (navigator.serviceWorker.controller) {
       return resolve()
    }
    // Or wait for the service worker to be in control (first page load)
    const controllerChangeHandler = (event) => {
      navigator.serviceWorker.removeEventListener('controllerchange', controllerChangeHandler)
      resolve()
    }
    navigator.serviceWorker.addEventListener('controllerchange', controllerChangeHandler)
  })
}

A variation (I didn't test it)

async function doSomethingOnFirstInstallation(something) {
    if (navigator.serviceWorker.controller) {
       return
    }

    const controllerChangeHandler = (event) => {
      navigator.serviceWorker.removeEventListener('controllerchange', controllerChangeHandler)
      something()
    }
    navigator.serviceWorker.addEventListener('controllerchange', controllerChangeHandler)
  })
}

My use case:

  • Page is waiting for:
    • Service worker installed & in control (controlled promise)
    • Service worker get & store some global data in indexeddb
  • Page exec a fetch request
  • Service worker handles fetch event & respond with data from indexeddb

@philipwalton
Copy link
Member Author

Correct me if I'm wrong, but from your use cases, it sounds like you're primarily interested in knowing when a SW is in control the very first time. It doesn't sound like you're wanting to run any code every page load that a particular SW is in control over the page.

To do that in workbox-window currently, you'd do something like this:

const wb = new Workbox('sw.js');
await wb.register();

wb.addEventListener('controlling', () => {
  // Run code as soon as the registered SW is in control of the page.
  // On subsequent page loads, this event handler will never run.
});

// Do other SW stuff here...

If we make this a controlling promise, it really won't change the code much:

const wb = new Workbox('sw.js');
await wb.register();

wb.controlling.then(() => {
  // Run code as soon as the registered SW is in control of the page.
  // On subsequent page loads, this event handler will never run.
});

// Do other SW stuff here...

The only benefit I see to a promise is:

  • async/await, but I always explains why I think that's problematic, since it may block subsequent code from running ever
  • Promise.all() on multiple events (but I've yet to hear anyone ask for that).

@madmoizo
Copy link

Consider a web app (SPA) which requires some data relative to its global context (translation map, select list, w/e). These data are requested on every page load.
SPA just needs to be sure the fetch event will be handled by the service worker (to fetch these data from a local cache (indexeddb,...) instead of over the network).

@philipwalton
Copy link
Member Author

philipwalton commented Jan 16, 2019

SPA just needs to be sure the fetch event will be handled by the service worker (to fetch these data from a local cache (indexeddb,...) instead of over the network).

Sure, but consider all cases after the first time a user installs a SW for your origin (which most of the future of the web will entail). In these cases a controlling promise (as you've proposed) would often resolve to the previous app version's SW. Is that what you want?

I think the 99% use case is:
Is the SW that my app code was released with currently controlling the page?

And in my experience, you only need a callback for that once it starts controlling. Future page loads can assume that the controlling code already ran (either via a previous page load, or via the SW's activate event). And if you really need code that runs on every page load but only if you're version of the SW is controlling the page, I'm not aware of any reliable way to do that without storing a SW version and sending a message to the SW to confirm the versions match. But I don't think Workbox could possibly do that (outside of a build tool).

Am I'm ignoring another use case?

@madmoizo
Copy link

madmoizo commented Jan 16, 2019

Is that what you want?

Yes, if I need the new one, I can handle service worker update and force a reload (or ask the user)

Am I'm ignoring another use case?

I don't know :( still waiting for a 1%-man to decrease the 99% use case to 98%!

Just kidding, you are probably right about the 99% use case but I'm sure workbox-window ambition is not to abstract 10 loc of a basic "register & check update"

@philipwalton philipwalton deleted the workbox-window branch February 22, 2019 22:21
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.

5 participants