-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Service worker removal #170918
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
Service worker removal #170918
Conversation
bkonyi
left a comment
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.
Drive by review.
| }); | ||
| } | ||
| } | ||
| } |
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.
These .js files need a trailing newline at the end of the file.
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.
done!
dev/bots/service_worker_test.dart
Outdated
|
|
||
| print('Waiting for cleanup worker to execute...'); | ||
| // The cleanup worker will force another reload. We wait for it to settle. | ||
| await Future<void>.delayed(const Duration(seconds: 5)); |
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.
Is there any other way to wait for the page to settle (e.g., a log message)? This is a rather long delay and we try to avoid using timers in tests as they can result in flaky behavior.
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.
I shortened it to 100 milliseconds. Here is the pr: #173609. Did you have another idea in mind?
dev/bots/service_worker_test.dart
Outdated
| ); | ||
| } finally { | ||
| await server?.stop(); | ||
| if (await cleanupWorkerFile.exists()) { |
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.
Nit: use the *Sync() file system APIs if you're only performing individual file operations. The asynchronous versions are actually slower unless you're doing a lot of operations in parallel.
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.
Done!
| ]); | ||
| } | ||
|
|
||
| /// A drop-in replacement for `package:test` expect that can run outside the |
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.
I think this comment is still valid.
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.
Added it back
| foundError(<String>[buffer.toString(), StackTrace.current.toString()]); | ||
| } | ||
|
|
||
| /// Returns a pretty-printed representation of [value]. |
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.
These docs are still valid (_prettyPrint and _indent).
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.
Kept these in
mdebbar
left a comment
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.
This test is failing and needs to be updated (or deleted):
flutter/packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart
Lines 1371 to 1380 in 4fb1042
| // Contains the same file hash for both `/` and the root index.html file. | |
| const String rootIndexHash = 'd41d8cd98f00b204e9800998ecf8427e'; | |
| expect( | |
| environment.outputDir.childFile('flutter_service_worker.js').readAsStringSync(), | |
| contains('"/": "$rootIndexHash"'), | |
| ); | |
| expect( | |
| environment.outputDir.childFile('flutter_service_worker.js').readAsStringSync(), | |
| contains('"index.html": "$rootIndexHash"'), | |
| ); |
| * @returns {Promise<ServiceWorker>} | ||
| * This method is no longer called by loadServiceWorker. | ||
| */ | ||
| async _getNewServiceWorker(serviceWorkerRegistration, serviceWorkerVersion) { |
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.
This function isn't used anymore. Can we delete it?
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.
yup!
| * @returns {Promise<void>} | ||
| */ | ||
|
|
||
| async _waitForServiceWorkerActivation(serviceWorker) { |
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.
Same
| const deletePromises = OLD_CACHE_NAMES.map((key) => self.caches.delete(key)); | ||
| await Promise.all(deletePromises); | ||
| } catch (e) { | ||
| console.warn('Failed to delete old caches:', e); |
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.
| console.warn('Failed to delete old caches:', e); | |
| console.warn('Failed to delete old service worker caches:', e); |
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.
done! thank you!
| }) | ||
| } | ||
| } catch (e) { | ||
| console.warn('Failed to navigate clients:', e); |
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.
| console.warn('Failed to navigate clients:', e); | |
| console.warn('Failed to navigate service worker clients:', e); |
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.
done thank you!
|
|
||
| for (const client of clients) { | ||
| if (client.url && 'navigate' in client) { | ||
| client.navigate(client.url); |
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.
I don't really understand what this does. Could you please explain what this does?
| () => runWebServiceWorkerTestWithGeneratedEntrypoint(headless: true), | ||
| () => runWebServiceWorkerTestWithBlockedServiceWorkers(headless: true), | ||
| () => runWebServiceWorkerTestWithCustomServiceWorkerVersion(headless: true), | ||
| () => runCleanupVerificationTest(headless: true), |
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.
Let's spell out "service worker":
| () => runCleanupVerificationTest(headless: true), | |
| () => runServiceWorkerCleanupTest(headless: true), |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Hey @salemiranloye! Is this still something you're planning on landing? |
|
@SydneyBao should be working on landing this! |
|
@mdebbar should we consider this abandoned and close or take some time to clean this up and land it? |
|
I was waiting for Salem to push his local changes, but he never did. I will submit a new pr soon. Sorry about the delay. |
|
Sorry for the delay. Here is my updated pr: #173609 |
|
Superseded by #173609 |
Introduce a self-cleaning service worker to replace the old one. Previous attempts (#170918, #173609) had (accidentally?) disabled the loading of the service worker in `flutter.js`. This PR preserves the logic for loading the service worker, and it prints a deprecation warning. Towards #156910 Closes #106225
Introduce a self-cleaning service worker to replace the old one. Previous attempts (flutter#170918, flutter#173609) had (accidentally?) disabled the loading of the service worker in `flutter.js`. This PR preserves the logic for loading the service worker, and it prints a deprecation warning. Towards flutter#156910 Closes flutter#106225
Creating a service worker to remove the old cached service worker then unregister itself since flutters service worker is not being maintained so now it is being deprecated
Related Issue: #156910 (comment)
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.