Repository navigation
Service worker preload support - #540
camillobruni wants to merge 78 commits into
Conversation
✅ Deploy Preview for webkit-speedometer-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for webkit-speedometer-preview ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
# Conflicts: # experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/test-invoker.mjs # experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/test-runner.mjs # resources/suite-runner.mjs # suites-experimental/javascript-wc-indexeddb/dist/resources.txt # suites-experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/step-runner.mjs # suites-experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/step-scheduler.mjs # suites-experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/test-invoker.mjs # suites-experimental/javascript-wc-indexeddb/dist/src/speedometer-utils/test-runner.mjs # suites-experimental/responsive-design/dist/resources.txt # suites-experimental/suites.mjs # suites-experimental/todomvc-dart-jaspr/dist/out-dart2js-O4/resources.txt # suites-experimental/todomvc-dart-jaspr/dist/out-dart2wasm-O2/resources.txt # suites-experimental/todomvc-localstorage/dist/base.css # suites-experimental/todomvc-localstorage/dist/index.css # suites-experimental/todomvc-localstorage/dist/resources.txt # suites-experimental/todomvc-localstorage/src/resources.txt # suites/charts/dist/resources.txt # suites/default-suites.mjs # suites/editors/dist/resources.txt # suites/newssite/news-next/dist/resources.txt # suites/newssite/news-next/public/resources.txt # suites/react-stockcharts/build/resources.txt # suites/todomvc/architecture-examples/angular-complex/dist/resources.txt # suites/todomvc/architecture-examples/angular/dist/resources.txt # suites/todomvc/architecture-examples/backbone-complex/dist/resources.txt # suites/todomvc/architecture-examples/backbone/dist/resources.txt # suites/todomvc/architecture-examples/jquery-complex/dist/resources.txt # suites/todomvc/architecture-examples/jquery/dist/resources.txt # suites/todomvc/architecture-examples/lit-complex/dist/resources.txt # suites/todomvc/architecture-examples/lit/dist/resources.txt # suites/todomvc/architecture-examples/preact-complex/dist/resources.txt # suites/todomvc/architecture-examples/preact/dist/resources.txt # suites/todomvc/architecture-examples/react-complex/dist/resources.txt # suites/todomvc/architecture-examples/react-redux-complex/dist/resources.txt # suites/todomvc/architecture-examples/react-redux/dist/resources.txt # suites/todomvc/architecture-examples/react/dist/resources.txt # suites/todomvc/architecture-examples/svelte-complex/dist/resources.txt # suites/todomvc/architecture-examples/svelte/dist/resources.txt # suites/todomvc/architecture-examples/vue-complex/dist/resources.txt # suites/todomvc/architecture-examples/vue/dist/resources.txt # suites/todomvc/big-dom-generator/dist/resources.txt # suites/todomvc/todomvc-css/dist/resources.txt # suites/todomvc/vanilla-examples/javascript-es5-complex/dist/resources.txt # suites/todomvc/vanilla-examples/javascript-es6-webpack-complex/dist/resources.txt # suites/todomvc/vanilla-examples/javascript-es6-webpack/dist/resources.txt # suites/todomvc/vanilla-examples/javascript-web-components-complex/dist/resources.txt # suites/todomvc/vanilla-examples/javascript-web-components/dist/resources.txt # tests/run-end2end.mjs
This fixes a race condition where SpeedometerReady would fire while _preloadResources was still in flight asynchronously. That caused the test runner to simultaneously invoke benchmarkClient.start(), leading to concurrent preloads that randomly caused the PRELOAD_SUITES message to resolve a STOP_PRELOADING promise incorrectly on slower browsers.
There was a problem hiding this comment.
Thanks a lot for working on this! I left some comments to the issues that I found in my first review iteration, so not a comprehensive review yet. Please let me know what you think!
One issue that I noticed while checking the deploy preview link is that the "Use service worker for resource preloading" checkbox inside the developer mode doesn't work correctly. For example I go to https://deploy-preview-540--webkit-speedometer-preview.netlify.app/?developerMode=true and check the "Use service worker for resource preloading" checkbox. And it still doesn't do any preloading step when the benchmark is started. Then I refresh the page (now that it has &preload=true in the url), and try again and it works then.
Also, I think it would be great to write a small explanation at the PR description about the problem this PR is solving.
And also, I'm not entirely sure about the commit standards of this repo exactly, but it would be a great if you can squash the fx / cleanup / pre-format commits before merging.
| .trim() | ||
| .split("\n") | ||
| .map((resourceUrl) => ({ | ||
| url: new URL(resourceUrl.trim(), suite.url).href, |
There was a problem hiding this comment.
This resolves each resources.txt entry against suite.url, but tests/unittests/resources.mjs resolves them against the directory of the resources.txt file. For Perf-Dashboard these differ, so all 144 entries resolve to public/v3/public/api/... and 404.
resources:suites/perf.webkit.org/resources.txturl:suites/perf.webkit.org/public/v3/index.html
I think this goes unnoticed because !response.ok returns 0.
I guess we can use suite.resources to resovle them?
There was a problem hiding this comment.
Done — parseSuiteResources now resolves against suite.resources.
| }); | ||
| }); | ||
|
|
||
| describe("isDefault", () => { |
There was a problem hiding this comment.
This looks like a duplicate of the isDefault describe block above. Probably a merge leftover?
There was a problem hiding this comment.
Removed duplicate.
| } | ||
|
|
||
| async _unregisterOldServiceWorkers() { | ||
| const existingRegistrations = await navigator.serviceWorker.getRegistrations(); |
There was a problem hiding this comment.
Hmm, navigator.serviceWorker is undefined in non-secure contexts. And the setup runs even with preload=false, so this throws and the page never reaches the ready state. I think it would be better to not touch the service owrker when the preload is off.
Also getRegistrations() returns every registration on the origin, so this also unregisters the workers of other benchmarks' or Speedometer versions. Could you filter it by scope?
There was a problem hiding this comment.
Done — guarded non-secure contexts, skipped SW setup when preload is off, and filtered unregistration by scope.
| return; | ||
| } | ||
|
|
||
| this._validateMessageRouting(data); |
There was a problem hiding this comment.
_pendingRequest switches to STOP as soon as it's posted, but PROGRESS/DONE messages already in flight from the SW then make this throw inside the listener. The same happens after a timeout. Request ids, with stale replies ignored, would fix it.
There was a problem hiding this comment.
Done — added monotonic requestId matching and silently ignore stale SW replies.
| const timeSec = (timeTakenMs / 1000).toFixed(2); | ||
| console.log(`Preloaded ${response.count} files (${sizeMB} MB transferred) in ${timeSec}s`); | ||
| } | ||
| this._preloadParams = params.toSearchParams(); |
There was a problem hiding this comment.
I think a preload that's stopped early resolves with null and still sets _preloadParams, so the next isCached() returns true on a partial cache. I think it's better to set it only on PRELOAD_DONE.
There was a problem hiding this comment.
Done — _preloadParams is now set only on PRELOAD_DONE and cleared on stop/teardown.
| if (!this._isStepping()) | ||
| this._developerModeContainer?.remove(); | ||
|
|
||
| await this._preloadResources(benchmarkConfigurator); |
There was a problem hiding this comment.
_startBenchmark only guards on isRunning(), and it looks like buttons stay enabled during a post-READY preload. Can we bail on PRELOADING too?
There was a problem hiding this comment.
Done — guarded start(), step(), and _startBenchmark() on PRELOADING and disabled start buttons during preload.
| import { SW_MESSAGES } from "./shared/sw-messages.mjs"; | ||
| import { params } from "./shared/params.mjs"; | ||
|
|
||
| const PRELOAD_TIMEOUT_MS = 60_000; |
There was a problem hiding this comment.
I don't know if this is a tight window on a slow connection, do you have any idea?
There was a problem hiding this comment.
idk, but 1 min seems like a lot to me 🤷
There was a problem hiding this comment.
Let's keep 60s for now and see how it scales on slower connections.
+1 🙂 IIUC, this should mostly affect the loading process, which is not timed. |
Resolve PR 540 review comments by resolving suite resources relative to suite.resources, correlating service worker messages by requestId, scoping service worker registration cleanup, and guarding non-secure contexts. Synchronize developer mode preload toggles and benchmark UI states while expanding unit and end-to-end test coverage.
Ensure Service Worker registration waits for active controller readiness, synchronize stopPreloading with preloadSuites via _activeStopPromise, and resolve suite.resources against self.location.href. Ensure the home section is visible during preloading, clear stepping state on completion, and add end-to-end coverage for enabling preload from the developer menu.
Replace verbose null and type guards across the Service Worker, preloader, configurator, and parameter parser with optional chaining, and remove redundant DOM element checks in PreloadStatusUpdater.
# Conflicts: # tests/index.html
Encapsulate benchmark error state checks behind a dedicated client helper to improve readability and consistency with other lifecycle state predicates.
Use isRunning() and isPreloading() within _setBenchmarkState to align with the rest of MainBenchmarkClient's state predicate patterns.
Encapsulate disabled state check for start buttons into a dedicated helper to match other state predicates on MainBenchmarkClient.
Extract cooperative preemption check into a helper method with documentation on when pending messages may be preempted.
|
Fixed the dev-mode preload checkbox; will add a PR description and squash commits before merge. |
No description provided.