core(installable-manifest): use devtools InstallabilityErrors #11745

adrianaixba · 2020-12-02T02:27:58Z

Use devtools InstallabilityErrors, to reduce the variability between devtools and the lighthouse report.

Follow ups:

modification of report: some descriptions/title changes here, but moving service worker audit & removing is-on-https in report(pwa): move service-worker to the pwa-optimized group #11798
smoke coverage for warn-not-offline-capable: Add smoke coverage to warn-not-offline-capable InstallabilityErrors #11826

Future action:

edit any error messages that could provide better actionability to LH users ->put on hold to communicate w devtools/installable folks
Possible future additions (not possible right now, I believe):
include image errors/warnings that aren't exported by devtools (new_audit: common PWA icon snafus #11762)

connorjclark · 2020-12-02T16:38:16Z

Since this is in draft state, can you specify which parts you'd like feedback on, and what things you already know needs more work?

brendankenny

a few comments on some things that immediately jump out at me :)

lighthouse-cli/test/smokehouse/test-definitions/offline-local/offline-expectations.js

lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa-expectations.js

brendankenny · 2020-12-04T01:03:07Z

lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js

-          score: 0,
-          details: {items: [jakeExpectations]},
-          explanation: /^Failures: .*short_name/,
+          score: 1,


does this mean "Manifest does not contain a 'name' or 'short_name' field" only requires one of name and short_name, so is passing now?

😆

a coupla weeks ago i spent a while looking at https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/installable/installable_manager.cc;l=651-655;drc=05dff50d9ce188e04ef796f0f1c28fbef09ace59 and my brain still doesn't make sense of it.

it seeems to say:

if (nameIsMissingOrEmpty && shortNameIsMissingOrEmpty) { errors.push('MANIFEST_MISSING_NAME_OR_SHORT_NAME') }

and how does && in the logic match up with a var named OR.

in fact it seemed so wrong that i figured it might be a bug. so adriana and I made a CL: https://chromium-review.googlesource.com/c/chromium/src/+/2530481
but the tests are all failureful.

so tbh i have no idea.

The fact that this makes sense to me now has me worried about my understanding... 😆

I think the MANIFEST_MISSING_NAME_OR_SHORT_NAME is just ambiguously named because when interpreting it as "manifest missing something - name or short name needed", which is consistent with my understanding that you need something to use as the name, is all works out. You definitely don't need both name and short_name.

It's not the case for installability that you need a short_name, right? I believe Lighthouse just did so because we thought it was a good idea for most apps, but now that we've switched I would expect the && to fail the tests and our result to change.

lighthouse-core/audits/installable-manifest.js

brendankenny · 2020-12-04T01:07:02Z

lighthouse-core/audits/installable-manifest.js

+    /** Error message explaining that a URL in the manifest contains a username, password, or port. */
+    'url-not-supported-for-webapk': `A URL in the manifest contains a username, password, or port`,
+    /** Error message explaining that the page is loaded in an incognito window. */
+    'in-incognito': `Page is loaded in an incognito window`,


we recommend users with busy extensions or important storage use incognito to test. Does this mean PWA testing won't work there?

yes this is accurate.

:/

we could choose to special-case this one...

we probably are special casing this "warn" error that's landing soon: https://chromium-review.googlesource.com/c/chromium/src/+/2567057

lighthouse-core/audits/installable-manifest.js

brendankenny · 2020-12-04T01:11:29Z

lighthouse-core/audits/installable-manifest.js

+    for (const err of installabilityErrors) {
+      // @ts-expect-error errorIds from protocol should match up against the strings dict
+      const matchingString = UIStrings[err.errorId];
+      // We only expect a `minimum-icon-size-in-pixels` errorArg[0] for two errorIds, currently.


this will throw if this statement becomes not true at some point in the future (new error type or whatever), so better to check this as well :)

yeaaaaah true

a brand new error id that our code doesn't handle will fall into L157.

an existing error id that our code handles could have an arg added to it, and we'd fall into L164 but the error message string wouldn't handle the values. i18n will throw. https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/i18n/i18n.js#L267

we could avoid errors if _str somehow provided how many arguments a formatted string has, and check if that is equal to errorArguments.length, and if not we fallback to a generic string (${errorId} ${errorArguments.join(', ')}). But _str doesn't provide # of args. We could do a string search for value# for each index in errorArguments as a substitute ...

@connorjclark What do you think about having something like this?

const errorArgumentsRegex = /{([^}]+)}/g; const UIStringArguments = matchingString.match(errorArgumentsRegex) || []; if (matchingString && value0) { errorMessages.push(str_(matchingString, {value0})); } else if (matchingString && err.errorArguments.length !== UIStringArguments.length) { // Matching string, but have the incorrect number of arguments for the message. errorMessages.push(str_(UIStrings.notEnoughArguments, {errorId: err.errorId})) } else if (matchingString) { errorMessages.push(str_(matchingString)); } else { errorMessages.push(str_(UIStrings.noErrorId)); }

Where the notEnoughArguments string would be the generic string you mentioned. Although it doesn't include the args in the string. Somewhat similar to how Devtools does it

looks good, that's a tricky regex. couple things

the first case is still susceptible to an error (if we get an unexpected number of args for an error we already handle).

the check for matchingString && err.errorArguments.length !== UIStringArguments.length should go first, that'd solve thing 1)

let's add a second arg to UIStrings.notEnoughArguments, so the string can attempt to be useful (at the risk of printing randomish data). I wonder if we even should use i18n here. @exterkamp the string would be ${errorId} ${errorArguments.join(', ') ... any point to send that to translators?

I suspect the "1 arg case" is fine forever, even if a "2 arg case" comes up still just duplicating a little code instead of making a generic "n arg case" is much better to read.

shane says let's avoid UI strings/i18n for the "notEnoughArguments" case

lighthouse-core/audits/installable-manifest.js

paulirish · 2020-12-04T08:54:07Z

lighthouse-core/audits/installable-manifest.js

+    /** Error message explaining that a URL in the manifest contains a username, password, or port. */
+    'url-not-supported-for-webapk': `A URL in the manifest contains a username, password, or port`,
+    /** Error message explaining that the page is loaded in an incognito window. */
+    'in-incognito': `Page is loaded in an incognito window`,


yes this is accurate.

:/

we could choose to special-case this one...

we probably are special casing this "warn" error that's landing soon: https://chromium-review.googlesource.com/c/chromium/src/+/2567057

lighthouse-core/audits/installable-manifest.js

paulirish · 2020-12-04T08:55:59Z

lighthouse-core/audits/installable-manifest.js

+    for (const err of installabilityErrors) {
+      // @ts-expect-error errorIds from protocol should match up against the strings dict
+      const matchingString = UIStrings[err.errorId];
+      // We only expect a `minimum-icon-size-in-pixels` errorArg[0] for two errorIds, currently.


yeaaaaah true

lighthouse-core/lib/i18n/i18n.js

lighthouse-core/test/results/sample_v2.json

lighthouse-cli/test/smokehouse/test-definitions/offline-local/offline-expectations.js

lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa-expectations.js

paulirish · 2020-12-04T09:09:18Z

lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js

-          score: 0,
-          details: {items: [jakeExpectations]},
-          explanation: /^Failures: .*short_name/,
+          score: 1,


😆

a coupla weeks ago i spent a while looking at https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/installable/installable_manager.cc;l=651-655;drc=05dff50d9ce188e04ef796f0f1c28fbef09ace59 and my brain still doesn't make sense of it.

it seeems to say:

if (nameIsMissingOrEmpty && shortNameIsMissingOrEmpty) { errors.push('MANIFEST_MISSING_NAME_OR_SHORT_NAME') }

and how does && in the logic match up with a var named OR.

in fact it seemed so wrong that i figured it might be a bug. so adriana and I made a CL: https://chromium-review.googlesource.com/c/chromium/src/+/2530481
but the tests are all failureful.

so tbh i have no idea.

patrickhulce · 2020-12-14T18:28:31Z

@adrianaixba is the status of this PR correct, awaiting a re-review from @paulirish ?

adrianaixba · 2020-12-14T18:43:55Z

@adrianaixba is the status of this PR correct, awaiting a re-review from @paulirish ?

One more pass would be nice :)

paulirish

lg!

brendankenny

sorry, last things :)

brendankenny · 2020-12-14T20:00:21Z

lighthouse-core/audits/installable-manifest.js

+      let matchingString;
+      try {
+        // @ts-expect-error errorIds from protocol should match up against the strings dict
+        matchingString = UIStrings[err.errorId];


This line is a potential typescript error, but it won't ever throw at runtime (it'll just be undefined). Can just do an if (matchingString === undefined) { check (though the compiler may complain about that too)

this doesn't seem like it got changed?

lighthouse-core/audits/installable-manifest.js

adrianaixba · 2020-12-14T22:36:05Z

is this easy to add to the non-offline smoke tests sites, too?

Created new issue for this #11826 :)

brendankenny · 2020-12-14T23:37:47Z

lighthouse-core/test/audits/installable-manifest-test.js

      artifacts.WebAppManifest = null;
      const context = generateMockAuditContext();

      return InstallableManifestAudit.audit(artifacts, context).then(result => {
        assert.strictEqual(result.score, 0);
-        assert.ok(result.explanation.includes('No manifest was fetched'), result.explanation);
+        const items = result.details.items;
+        assert.ok(items[0].reason.formattedDefault.includes('no manifest'));


Suggested change

assert.ok(items[0].reason.formattedDefault.includes('no manifest'));

expect(items[0].reason).toBeDisplayString(/no manifest/);

this is the "proper" way to test the localized version of the string (or i18n.getFormatted(items[0].reason, 'en-US').includes('no manifest')) Mostly the same in practice and in theory, but

doesn't reach into the internal representation (allowing for easier i18n refactors in the future) and

is testing the final string coming out of the i18n pipeline, so it's closer to what will be in the actual report (formattedDefault is the fallback in case that pipeline fails)

mainly I expect people will be copy/pasting from these tests in the future so it's good to do it the slightly better way.

this is very clean, i like it 👀

lighthouse-core/test/audits/installable-manifest-test.js

brendankenny · 2020-12-14T23:57:30Z

lighthouse-core/test/audits/installable-manifest-test.js

@@ -35,12 +35,14 @@ describe('PWA: webapp install banner audit', () => {
  describe('basics', () => {


really a lot of these tests could be dropped now since most of them are just testing are errorIds mapped to strings in the items, but there's not any harm in leaving them in place

brendankenny

a few more things, mainly still just test coverage

brendankenny · 2020-12-15T00:01:45Z

lighthouse-core/audits/installable-manifest.js

-  description: 'Browsers can proactively prompt users to add your app to their homescreen, ' +
-    'which can lead to higher engagement. ' +
-    '[Learn more](https://web.dev/installable-manifest/).',
+  'description': `Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper Service Worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).`,


Suggested change

'description': `Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper Service Worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).`,

'description': `Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper service worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).`,

https://web.dev/installable-manifest

we should update the web.dev article soon to include the content from the other service-worker audits

brendankenny · 2020-12-15T00:13:18Z

lighthouse-core/audits/installable-manifest.js

+  'url-not-supported-for-webapk': `A URL in the manifest contains a username, password, or port`,
+  /** Error message explaining that the page is loaded in an incognito window. */
+  'in-incognito': `Page is loaded in an incognito window`,
+  // TODO: perhaps edit this message to make it more actionable for LH users


This will get lost in here, so should this be filed as an issue or is it covered enough now that it could be removed?

done! #11830 Yeah these are messages that we might want to communicate with the installability/devtools folks about, to get a clearer definition of for LH

lighthouse-core/audits/installable-manifest.js

lighthouse-core/test/audits/installable-manifest-test.js

brendankenny

LGTM!

adrianaixba added 5 commits November 11, 2020 16:52

wip, todo: better testing, and cleanup

5154ca6

comments

201cf10

remove <M82 test

d6b1187

cleanups

88a9dd7

fix smoke tess

e0833c0

google-cla bot added the cla: yes label Dec 2, 2020

adrianaixba changed the title ~~core(installable-manifest): replace installability checks with devtools InstallabilityErrors~~ core(installable-manifest): use devtools InstallabilityErrors Dec 2, 2020

Merge branch 'master' into dt-installable

4a6e8a8

vercel bot deployed to Preview December 2, 2020 03:14 View deployment

adrianaixba added 2 commits December 2, 2020 16:52

remove manifestValues assessment

8755306

errors but better things w paul :)

4f03976

vercel bot deployed to Preview December 3, 2020 23:47 View deployment

adrianaixba added 2 commits December 3, 2020 18:10

sample json & u18n fix w paul

a3e1706

cleanups

6d24771

vercel bot deployed to Preview December 4, 2020 00:21 View deployment

paulirish mentioned this pull request Dec 4, 2020

new_audit: common PWA icon snafus #11762

Closed

brendankenny reviewed Dec 4, 2020

View reviewed changes

tests cleanup, need to fix stack packs test

ef8239a

vercel bot deployed to Preview December 4, 2020 02:56 View deployment

new error & message

86b6035

vercel bot deployed to Preview December 4, 2020 03:07 View deployment

cleanups

c0da38f

vercel bot deployed to Preview December 4, 2020 03:18 View deployment

paulirish mentioned this pull request Dec 4, 2020

tests: cron to check for relevant chromium changes #11763

Merged

paulirish reviewed Dec 4, 2020

View reviewed changes

tests cleanups etc

78bb8a5

vercel bot deployed to Preview December 4, 2020 18:51 View deployment

cleanups

29f37d5

vercel bot deployed to Preview December 4, 2020 18:58 View deployment

vercel bot deployed to Preview December 11, 2020 19:51 View deployment

update devtools #500000

0ece709

vercel bot deployed to Preview December 11, 2020 20:26 View deployment

patrickhulce assigned paulirish and unassigned Beytoven Dec 14, 2020

paulirish approved these changes Dec 14, 2020

View reviewed changes

brendankenny requested changes Dec 14, 2020

View reviewed changes

remove try block

8dd7f0d

vercel bot deployed to Preview December 14, 2020 22:08 View deployment

adrianaixba mentioned this pull request Dec 14, 2020

Add smoke coverage to warn-not-offline-capable InstallabilityErrors #11826

Closed

adrianaixba requested a review from brendankenny December 14, 2020 23:37

brendankenny reviewed Dec 14, 2020

View reviewed changes

brendankenny reviewed Dec 15, 2020

View reviewed changes

unit-test cleanup

1f6938d

vercel bot deployed to Preview December 15, 2020 00:26 View deployment

cleanups

60ee667

vercel bot deployed to Preview December 15, 2020 01:07 View deployment

adrianaixba mentioned this pull request Dec 15, 2020

Modify Devtools InstallabilityErrors to be more actionable for LH users #11830

Closed

DreamTechBDshop referenced this pull request Dec 15, 2020

misc: fix npm auth in nightly release (#11810)

88cb020

brendankenny approved these changes Dec 15, 2020

View reviewed changes

brendankenny added land-when-ci-is-green and removed waiting4reviewer labels Dec 15, 2020

devtools-bot merged commit 3d90a59 into master Dec 15, 2020

devtools-bot deleted the dt-installable branch December 15, 2020 16:00

devtools-bot removed the land-when-ci-is-green label Dec 15, 2020

brendankenny mentioned this pull request Dec 15, 2020

start_url fetched via an "offline" SW not considered good enough #10237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(installable-manifest): use devtools InstallabilityErrors #11745

core(installable-manifest): use devtools InstallabilityErrors #11745

adrianaixba commented Dec 2, 2020 •

edited

Loading

connorjclark commented Dec 2, 2020

brendankenny left a comment

brendankenny Dec 4, 2020

paulirish Dec 4, 2020

patrickhulce Dec 14, 2020 •

edited

Loading

brendankenny Dec 4, 2020

paulirish Dec 4, 2020

brendankenny Dec 4, 2020

paulirish Dec 4, 2020

connorjclark Dec 7, 2020

adrianaixba Dec 8, 2020 •

edited by connorjclark

Loading

connorjclark Dec 8, 2020 •

edited

Loading

connorjclark Dec 9, 2020

paulirish Dec 4, 2020

paulirish Dec 4, 2020

paulirish Dec 4, 2020

patrickhulce commented Dec 14, 2020

adrianaixba commented Dec 14, 2020 •

edited

Loading

paulirish left a comment

brendankenny left a comment

brendankenny Dec 14, 2020

adrianaixba commented Dec 14, 2020

brendankenny Dec 14, 2020

adrianaixba Dec 15, 2020

brendankenny Dec 14, 2020

brendankenny left a comment

brendankenny Dec 15, 2020 •

edited

Loading

brendankenny Dec 15, 2020

adrianaixba Dec 15, 2020

brendankenny left a comment

	assert.ok(items[0].reason.formattedDefault.includes('no manifest'));
	expect(items[0].reason).toBeDisplayString(/no manifest/);

		@@ -35,12 +35,14 @@ describe('PWA: webapp install banner audit', () => {
		describe('basics', () => {

	'description': `Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper Service Worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).`,
	'description': `Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper service worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).`,

core(installable-manifest): use devtools InstallabilityErrors #11745

core(installable-manifest): use devtools InstallabilityErrors #11745

Conversation

adrianaixba commented Dec 2, 2020 • edited Loading

connorjclark commented Dec 2, 2020

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianaixba Dec 8, 2020 • edited by connorjclark Loading

Choose a reason for hiding this comment

connorjclark Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Dec 14, 2020

adrianaixba commented Dec 14, 2020 • edited Loading

paulirish left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianaixba commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

adrianaixba commented Dec 2, 2020 •

edited

Loading

patrickhulce Dec 14, 2020 •

edited

Loading

adrianaixba Dec 8, 2020 •

edited by connorjclark

Loading

connorjclark Dec 8, 2020 •

edited

Loading

adrianaixba commented Dec 14, 2020 •

edited

Loading

brendankenny Dec 15, 2020 •

edited

Loading