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.

Sign up for GitHub

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: workbox-window method to trigger SW update check #2136

Merged
merged 4 commits into from
Aug 1, 2019
Merged

feat: workbox-window method to trigger SW update check #2136

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e48cded
feat: add update method
jaulz Jul 20, 2019
9e62911
fix: clean up update method
jaulz Jul 23, 2019
a617931
test: add test for update
Jul 25, 2019
364c488
test: add test for update
Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/workbox-window/src/Workbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,27 @@ class Workbox extends WorkboxEventTarget {
return this._registration;
}

/**
* Checks for updates of the registered service worker.
*/
async update() {
if (!this._registration) {
if (process.env.NODE_ENV !== 'production') {
logger.error('Cannot update a Workbox instance without ' +
'being registered. Register the Workbox instance first.');
}
return;
}

// Reset the registration time and update count so it's not treated as
// external in the `this._onUpdateFound` heuristic.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure resetting these here is what we want to do. Based on how I describe the intention of the "external" events in the workbox-window guide, an update found via calling update() is (for all intents and purposes) the same as an update found via the app being updated in a new tab.

Do you disagree? Based on what you said in this comment I thought you wanted to treat them the same way: #2130 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it now but I don't understand why an update via update() is the same as an update via a new tab? Isn't the update eventually the same as a register() which is called after the page has been loaded?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the update eventually the same as a register() which is called after the page has been loaded?

Not quite. The difference is register() is always called first, and after the register() call you either have an update or you don't. But when you have an update from the register() call (in the same tab), it's always for a SW that is older (or the same age) as the code running on the window.

But if an update is found when calling update() (or .register() in another tab in the future), it means the updated SW is newer than the code running on the page, which means there could potentially be conflicts, depending on how old the code running on the page is.

this._registrationTime = performance.now();
this._updateFoundCount = 0;

// Try to update registration
await this._registration.update();
}

/**
* Resolves to the service worker registered by this instance as soon as it
* is active. If a service worker was already controlling at registration
Expand Down