Skip to content

Conversation

@salemiranloye
Copy link
Contributor

@salemiranloye salemiranloye commented Jun 20, 2025

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.

@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine related. See also e: labels. platform-web Web applications specifically labels Jun 20, 2025
@salemiranloye
Copy link
Contributor Author

@ditman

@bkonyi bkonyi requested a review from mdebbar June 23, 2025 14:38
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

Drive by review.

});
}
}
}
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

done!


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));
Copy link
Contributor

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.

Copy link
Contributor

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?

);
} finally {
await server?.stop();
if (await cleanupWorkerFile.exists()) {
Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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].
Copy link
Contributor

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Kept these in

@matanlurey matanlurey removed their request for review June 23, 2025 15:55
Copy link
Contributor

@mdebbar mdebbar left a 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):

// 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) {
Copy link
Contributor

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?

Copy link
Contributor

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) {
Copy link
Contributor

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.warn('Failed to delete old caches:', e);
console.warn('Failed to delete old service worker caches:', e);

Copy link
Contributor

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.warn('Failed to navigate clients:', e);
console.warn('Failed to navigate service worker clients:', e);

Copy link
Contributor

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);
Copy link
Contributor

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),
Copy link
Contributor

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":

Suggested change
() => runCleanupVerificationTest(headless: true),
() => runServiceWorkerCleanupTest(headless: true),

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@bkonyi
Copy link
Contributor

bkonyi commented Jul 25, 2025

Hey @salemiranloye! Is this still something you're planning on landing?

@ditman
Copy link
Member

ditman commented Jul 28, 2025

@SydneyBao should be working on landing this!

@bkonyi
Copy link
Contributor

bkonyi commented Aug 11, 2025

@mdebbar should we consider this abandoned and close or take some time to clean this up and land it?

@SydneyBao
Copy link
Contributor

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.

@SydneyBao
Copy link
Contributor

Sorry for the delay. Here is my updated pr: #173609

@mdebbar
Copy link
Contributor

mdebbar commented Aug 14, 2025

Superseded by #173609

@mdebbar mdebbar closed this Aug 14, 2025
github-merge-queue bot pushed a commit that referenced this pull request Oct 17, 2025
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
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels. platform-web Web applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants