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

Define WorkboxGlobalScope in workbox-precaching #2119

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

jeffposnick
Copy link
Contributor

R: @philipwalton

I'm not 100% sure that this is the right way to do things, so please review this with skepticism, but this definition is the cleanest way I could come up with to support using self.__WB_MANIFEST inside of a TypeScript swSrc file.

The usage ends up looking like:

import {precacheAndRoute, WorkboxGlobalScope} from 'workbox-precaching/precacheAndRoute';

declare const self: WorkboxGlobalScope;

precacheAndRoute(self.__WB_MANIFEST);

Maybe we should have a separate WorkboxGlobalScope declaration inside of workbox-core instead, which would presumably make it cleaner if we ever added in custom global properties that weren't related to workbox-precaching?

Copy link
Member

@philipwalton philipwalton left a comment

Choose a reason for hiding this comment

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

Yeah, In think it would be cleaner to add this to workbox-core/types (and possibly more future-friendly, as you say).

BTW, I spent a bit of time trying to make this work without the user having to import anything, and I couldn't make it work (without referencing self.__WB_MANIFEST in the source, which I don't think we want). I think it could work if the SW global was properly set by TypeScript, but I guess we'll have to wait and see.

@philipwalton
Copy link
Member

E.g. lit-html does something similar, but I believe that's only working because they're referencing the property in their source: https://github.com/Polymer/lit-html/blob/47a8af4805d775614fe075975eba2c3f6e521a04/src/lit-html.ts#L50-L54

@jeffposnick
Copy link
Contributor Author

Thanks for the tip regarding lit-html's usage. I think the magic here is the declare global {} wrapper around the interface.

With this latest commit, it looks like it just adds in support for the __WB_MANIFEST property as a side-effect of importing one of the precaching modules, and there's not requirement that I set self.__WB_MANIFEST to any value in the TypeScript source.

Given that this approach works, I'm thinking it makes more sense to keep it scoped tightly to the actual modules in which the additional properties will actually be used, rather than in workbox-core. If we need to extend ServiceWorkerGlobalScope with new properties in multiple places I believe that they're all considered additive, rather than overwriting each other.

@philipwalton
Copy link
Member

I see, so you're thinking of doing this but still expecting users to have to do this in their own code, correct?

declare var self: ServiceWorkerGlobalScope;

That seems fine to me, but note it's probably the case that many Workbox users wouldn't otherwise have to do declare var self: ServiceWorkerGlobalScope since they're likely not using self anywhere (they're using Workbox APIs).

Given that I wonder if it makes sense to also extend WorkerGlobalScope?

@coveralls
Copy link

coveralls commented Jul 17, 2019

Coverage Status

Coverage remained the same at 77.688% when pulling 4665c0d on workbox-global-scope into bd6edce on master.

@jeffposnick
Copy link
Contributor Author

I've switched to extending WorkerGlobalScope.

@workbox-pr-bot
Copy link
Collaborator

PR-Bot Size Plugin

Changed File Sizes

File Before After Change GZipped
packages/workbox-strategies/build/workbox-strategies.prod.js 4.10 KB 4.78 KB +16% 1.20 KB ☠️
packages/workbox-window/build/workbox-window.dev.umd.js 31.88 KB 31.88 KB -0% 8.10 KB

New Files

No new files have been added.

All File Sizes

View Table
File Before After Change GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js 3.86 KB 3.86 KB 0% 1.59 KB
packages/workbox-broadcast-update/build/workbox-broadcast-update.prod.js 1.92 KB 1.92 KB 0% 959 B
packages/workbox-build/build/_types.js 41 B 41 B 0% 61 B
packages/workbox-build/build/index.js 3.30 KB 3.30 KB 0% 1.31 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js 594 B 594 B 0% 354 B
packages/workbox-cli/build/app.js 4.15 KB 4.15 KB 0% 1.64 KB
packages/workbox-cli/build/bin.js 940 B 940 B 0% 502 B
packages/workbox-core/build/workbox-core.prod.js 5.94 KB 5.94 KB 0% 2.46 KB
packages/workbox-expiration/build/workbox-expiration.prod.js 2.94 KB 2.94 KB 0% 1.27 KB
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js 1.95 KB 1.95 KB 0% 913 B
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js 660 B 660 B 0% 324 B
packages/workbox-precaching/build/workbox-precaching.prod.js 4.25 KB 4.25 KB 0% 1.70 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js 1.57 KB 1.57 KB 0% 797 B
packages/workbox-routing/build/workbox-routing.prod.js 3.41 KB 3.41 KB 0% 1.47 KB
packages/workbox-strategies/build/workbox-strategies.prod.js 4.10 KB 4.78 KB +16% 1.20 KB ☠️
packages/workbox-streams/build/workbox-streams.prod.js 1.46 KB 1.46 KB 0% 720 B
packages/workbox-sw/build/workbox-sw.js 1.34 KB 1.34 KB 0% 747 B
packages/workbox-webpack-plugin/build/generate-sw.js 3.13 KB 3.13 KB 0% 1.22 KB
packages/workbox-webpack-plugin/build/index.js 349 B 349 B 0% 255 B
packages/workbox-webpack-plugin/build/inject-manifest.js 4.67 KB 4.67 KB 0% 1.59 KB
packages/workbox-window/build/workbox-window.dev.umd.js 31.88 KB 31.88 KB -0% 8.10 KB
packages/workbox-window/build/workbox-window.prod.umd.js 4.46 KB 4.46 KB 0% 1.84 KB

Workbox Aggregate Size Plugin

3.49KB gzip'ed (23% of limit)
7.8KB uncompressed

@jeffposnick jeffposnick merged commit 2ceb404 into master Jul 18, 2019
@jeffposnick jeffposnick deleted the workbox-global-scope branch July 18, 2019 14:25
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.

4 participants