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 serviceworker #234

Merged
merged 14 commits into from
Nov 8, 2018
Merged

Add a serviceworker #234

merged 14 commits into from
Nov 8, 2018

Conversation

developit
Copy link
Collaborator

Still a few bits to polish, but it works. Same tenchnique we have for workers.

return true;
});
}
\ await Promise.all(serviceWorkers.map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Syntax error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea how this got in there. We don't have an eslint config anymore though, should have caught this.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@jakearchibald
Copy link
Collaborator

sigh I broke the typings rather than fixing them. Will push a fix when I figure it out.

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

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

I approve Jason's changes here. Someone else should review mine 😀

@jakearchibald
Copy link
Collaborator

@developit during dev I'm getting a vendor~ file which breaks the service worker logic. Any idea where this comes from? I'm happy to code around it, I just want to understand why it's there first.

@jakearchibald
Copy link
Collaborator

I'm going to do more UI work around this, but I'll do that against one of the UI branches I already have to avoid conflicts.

@developit
Copy link
Collaborator Author

Weird, no idea what vendor~ would be. It sounds sortof like a chunk name? But we aren't emitting a vendor chunk. FWIW I'm not seeing it.

Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

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

Mostly nits and documentation from my side

config/auto-sw-plugin.js Show resolved Hide resolved
@@ -153,6 +154,12 @@ async function processSvg(blob: Blob): Promise<HTMLImageElement> {
return blobToImg(new Blob([newSource], { type: 'image/svg+xml' }));
}

async function getOldestServiceWorker() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

“Oldest” seems a bit weird. “MostRecent”?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those are opposites 😄. Would you prefer LeastRecent? I thought "oldest" was an easier way to say that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I understand now. It’s kinda weird to classify them in a temporal way.

From looking at the usage, you want the one that is “closest” to being active, no? So maybe something like getMostActiveServiceWorker()?

🚲🏚

// In production, this entire condition is removed.
if (process.env.NODE_ENV === 'development') {
// In production, install a Service Worker
if (process.env.NODE_ENV !== 'production') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this change make ServiceWorker not install in development?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure what happened with GH's comments but this line isn't in the current source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LOL

'process-',
];

const prefixMatches = buildAssets.filter(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit: Put this in a function like assetsWithPrefix(...prefix: string): string[]?

@jakearchibald jakearchibald merged commit 7d42d4f into master Nov 8, 2018
jakearchibald added a commit that referenced this pull request Nov 8, 2018
@jakearchibald
Copy link
Collaborator

Oops, wasn't meant to merge this one without addressing the nits. Will make those changes on master.

alisaitbilgi pushed a commit to alisaitbilgi/squoosh that referenced this pull request Feb 19, 2019
* Add a serviceworker

* rename + fix random extra character

* Fixing worker typings

* Fixing types properly this time.

* Once of those rare cases where this matters.

* Naming the things.

* Move registration to the app (so we can use snackbar later)

* Moving SW plugin later so it picks up things like HTML

* MVP service worker

* Two stage-service worker

* Fix prerendering by conditionally awaiting Custom Elements polyfill.

* Fix icon 404's

* add doc comment to autoswplugin

* Fix type
@developit developit deleted the service-worker branch December 8, 2020 01:52
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