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

Delete old caches on activate. #634

Merged
merged 3 commits into from
Jul 16, 2018

Conversation

tulioag
Copy link
Contributor

@tulioag tulioag commented Jul 11, 2018

Copy link

@vlukashov vlukashov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tulioag)


src/main/webapp/sw.js, line 2 at r1 (raw file):

// Important: update the version each time you change any of the files listed below
var version = 4;

Should it now become 5, not 1 (to avoid possible overlaps with the first "version 1")?


src/main/webapp/sw.js, line 2 at r1 (raw file):

// Important: update the version each time you change any of the files listed below
var version = 1;

nit: var and const used in the same file. For consistency it would be better to use the es5 syntax all over (i.e. replace all vars with either lets or consts)


src/main/webapp/sw.js, line 7 at r1 (raw file):

var offlineAssets = [offlinePage, 'manifest.json',
  'images/offline-login-banner.jpg'
]

nit: missing semicolon


src/main/webapp/sw.js, line 14 at r1 (raw file):

  return caches.open(CACHE_NAME)
    .then(function(cache) {
      cache.addAll(offlineAssets);

The return value of cache.addAll() is ignored. Is that intentional? That can cause the service worker to become installed before the cache is ready (i.e. chance of a race condition).


src/main/webapp/sw.js, line 14 at r1 (raw file):

    .then(function(cache) {
      cache.add(manifest);
      cache.add(offlinePage);

Is that intentional that the cache busting technique from https://googlechrome.github.io/samples/service-worker/custom-offline-page/ is not used here?


src/main/webapp/sw.js, line 26 at r1 (raw file):

            // If this cache name isn't present in the array of "expected" cache names,
            // then delete it.
            console.log('Deleting out of date cache:', cacheName);

Do we really need a console log statement?

Copy link
Contributor Author

@tulioag tulioag left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @vlukashov and @tulioag)


src/main/webapp/sw.js, line 2 at r1 (raw file):

Previously, vlukashov (Viktor Lukashov) wrote…

Should it now become 5, not 1 (to avoid possible overlaps with the first "version 1")?

Done.


src/main/webapp/sw.js, line 14 at r1 (raw file):

Previously, vlukashov (Viktor Lukashov) wrote…

Is that intentional that the cache busting technique from https://googlechrome.github.io/samples/service-worker/custom-offline-page/ is not used here?

Added cache busting.


src/main/webapp/sw.js, line 14 at r1 (raw file):

Previously, vlukashov (Viktor Lukashov) wrote…

The return value of cache.addAll() is ignored. Is that intentional? That can cause the service worker to become installed before the cache is ready (i.e. chance of a race condition).

Thanks for catiching that. The code was refactored due to adding cache bust


src/main/webapp/sw.js, line 26 at r1 (raw file):

Previously, vlukashov (Viktor Lukashov) wrote…

Do we really need a console log statement?

Removed

Copy link

@vlukashov vlukashov left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

2 participants