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

core(pwa): remove works-offline and offline-start-url audits #11806

Merged
merged 16 commits into from
Dec 15, 2020

Conversation

paulirish
Copy link
Member

Writing and maintaining these audits (well, the gatherers) was ... exciting. Over the past 4 years, I believe we changed our strategy for offline-start-url a good 5 times. 🤣

But now that we rely on the installabilityErrors from the protocol (#8223 #10515), we don't need to have custom functional tests.

And now that load-fast-enough-for-pwa has been removed (#11764), there is no longer a "Fast and reliable" group within PWA. (This PR complements #11745 which revamps installable-manifest and #11798 which makes service-worker hidden)

This PR removes the audits, gatherers, icon & gauge handling, and remaining references.


Look of the new PWA section with the 3 states:

image

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

+22 -2246 lines in PR 😍

@@ -605,35 +605,29 @@
.lh-gauge--pwa__logo--primary-color {
fill: #304FFE;
}
/* .dark .lh-gauge--pwa__logo--primary-color {
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

heh. i didn't love how the PWA logo looked in darkmode so i experimented with changes.. ultimately they weren't good enough to keep. thx for the catch.

lighthouse-core/config/default-config.js Show resolved Hide resolved
lighthouse-core/config/default-config.js Show resolved Hide resolved
lighthouse-core/config/default-config.js Show resolved Hide resolved
@brendankenny
Copy link
Member

But now that we rely on the installabilityErrors from the protocol (#8223 #10515), we don't need to have custom functional tests.

Not equivalent checks, of course :) start-url has been a problem, but works-offline won't have an equivalent for a while.

What are you thinking of for a timeline? With #11745 we'll have just the fetch handler check. In Chrome 93 (in August) an offline check becomes part of the installability requirements so we'll have one again. Turn on warn-not-offline-capable as a warning sometime in between now and August (like when it reaches stable)? Use the warning as a requirement in LH v8 so the big PWA change is aligned with our major release? Or just match Chrome installability and be ok with changes?

@paulirish
Copy link
Member Author

But now that we rely on the installabilityErrors from the protocol (#8223 #10515), we don't need to have custom functional tests.

Not equivalent checks, of course :) start-url has been a problem, but works-offline won't have an equivalent for a while.

yes. truth.

What are you thinking of for a timeline?

basic plan is shipping this in LH 7, yeah. :)

With #11745 we'll have just the fetch handler check. In Chrome 93 (in August) an offline check becomes part of the installability requirements so we'll have one again.

warn-not-offline-capable is now on by default in chrome 89 (current canary): https://chromium-review.googlesource.com/c/chromium/src/+/2580668 (thus our CI breakages last week)

so all canary folk have the functional offline check and it reports through to us. 👍

oh and if you didn't spot it.. #11745 now adds an audit warning when warn-not-offline-capable is present.

anyone using this lighthouse with chrome stable will yeah have a little gap as nothing is doing a functional offline test (LH or crbug.com/965802). (fwiw, m89 ships to stable ~march 2nd) But I don't see that as a serious problem. It would also be a problem for folks writing their own service worker and where Lighthouse would be surfacing a problem they wouldn't find themselves.

Or just match Chrome installability and be ok with changes?

Thats exactly where my priorities are yeah.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

cc @connorjclark if you still want to take a look

@paulirish paulirish merged commit 852e79a into master Dec 15, 2020
@paulirish paulirish deleted the dropofflineaudits branch December 15, 2020 19:46
ptim added a commit to gizmag/AutoWebPerf that referenced this pull request Mar 17, 2021
ptim added a commit to gizmag/AutoWebPerf that referenced this pull request Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants