Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: use org name in header #143
feat: use org name in header #143
Changes from 9 commits
4d59acf
fcfda4a
d449d8e
12c0158
4ef776c
364c55c
5f6c38f
8152967
7faad19
30d7066
c459465
363dcd7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async this instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this?
self.addEventListener("install", async (event) => {
not sure why thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange that you do self invoking functions to do async. Just wrap the entire callback inside an async function like we discussed here and then you can await them like normal.
Also change catch to try catch. And don't use thenable, use async await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The self invoking functions are needed because event's
waitUntil()
method receives a promise as a parameter - not a function that returns a promise, (see reference). I can only imagine that your idea was to remore the use ofwaitUntil
entirely, for instance:instead of:
The difference between these approaches is that with
waitUntil()
adding assets to cache becomes an asynchronous task, which would allow the task to be completed even after returning inside the event handler. During install this is not a problem, however in a fetch event I want the service worker to return whatever is in cache, but fire an asynchronous task to update the cache with a network response. If we removewaitUntil()
and just await the network response, we would have to wait for network response to return whatever is in cache, seriously compromising speed in very slow networks.Here is a QA using
waitUntil()
to return from cache and asynchronously update cache with network response, in a simulated 10kbps network speed:10kbps-network.webm
Here is a QA awaiting the network response, in a simulated 10kbps network speed:
10kbps-network-await.webm
Here are some additional references to clarify:
What does event.waitUntil do in service worker and why is it needed?
JavaScript how to use event.waitUntil() to wait for an async function call
Should I use event.waitUntil() or await fetch() or just fetch()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay seems like you did your research so let's do what you think is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was good feedback, it's very important that we understand each others code practices, as well as document weird patterns. Thanks.