Introduces isPWA boolean by ebidel · Pull Request #2032 · GoogleChrome/lighthouse

ebidel · 2017-04-18T20:45:21Z

Part of #1937.

Took at stab at this. Right now, it's based off of the pwa category score being 100. Nothing more.

Also added a small UI check if you're "certified". We don't have to do that but I figured people might want to see that in a report.

patrickhulce · 2017-04-18T20:56:29Z

short and sweet I like it, any thoughts on exposing this at the toplevel object vs. in the category? I kinda like it contained within the category but that does mean every consumer has to do results.reportCategories.find(cat => cat.id === 'pwa').isPwa

ebidel · 2017-04-18T20:58:37Z

Yea, it might be easy for consumers to check a top level prop rather than a buried obj.

Either is fine with me.

ebidel · 2017-04-18T22:47:04Z

isPWA is also top level now. Kept it part of the category too so the renderer UI can access it easily instead of poking back at the report. Not thrilled about the duplication though.

paulirish

Everything in here is looking on point. V nice.

paulirish · 2017-04-21T03:57:49Z

lighthouse-core/report/v2/renderer/report-renderer.js

+    const element = this._populateScore(
+        tmpl, score, 'numeric', category.name, category.description);
+    if (category.isPWA) {
+      const titleEl = element.querySelector('.lh-score__title');


extract this block into a separate method?

paulirish · 2017-04-21T04:33:57Z

I want to discuss how we represent the PWA score + passing threshold. My take is we want to reward only the best best apps with a score of 100, to incentivize performance improvements. So we should set the bar for isPWA passing a bit lower.

A wild strawman proposal appears!

Passing isPWA ✓

PWA score of 70, isPWA ✓: All PWA audits as passing. (FastEnough audit is passing). The TTI is scoring a 70/100.✜.
PWA score of 85, isPWA ✓: Same, but TTI is scoring 85/100.
PWA score of 100, isPWA ✓: Same, but TTI is scoring 100/100.

Failing isPWA ✘

PWA score of ~63, isPWA ✘: All PWA audits as passing, except for FastEnough audit, which fails as TTI is scoring < 70.
PWA scores of 63.8 - 69.9: Impossible. Once you're failing one of the PWA booleans, you're losing full points for that audit. (Currently we're at 11 audits, so each fail is worth ~6.3 pts, assuming even weighting.)

TTI Scoring

✜ TTI scoring has come up a lot, but one solution is: 70/100 is the score for whatever the "fast enough" definition of the baseline checklist says. (Currently its 10s, discussions around it moving to 5s). For example, that scoring curve could look like this: https://www.desmos.com/calculator/whus3whb2i (median at 15.7s, podr at 2s). 70% score at 10s. 80% score at 7.5s. 90% at 5.1s. (All the disclaimers about TTI changing apply here)

what do ya'll think?

patrickhulce · 2017-04-21T16:04:06Z

Lots to parse in there @paulirish :) but I like the idea of basically saying your final PWA score is min(pwa.score, timeToInteractive.score) but have the boolean still be based on pwa.score === 100

paulirish · 2017-04-22T00:47:14Z

@brendankenny is gonna iterate on this. we discussed a solution and with some slight changes this will be goooooooood.

ebidel · 2017-04-25T00:16:27Z

@brendankenny are you going add to this PR? Not sure what was discussed :)

brendankenny · 2017-04-25T01:40:30Z

yeah, I'm going to adapt it. Plan is basically just a new thing in config to make the check more detailed but also separate from category score. Should hit most of what we want and give us flexibility if we decide to use a different approach later

ebidel · 2017-05-16T03:18:03Z

@brendankenny do you think we'll land this for IO?

brendankenny · 2017-05-16T20:07:57Z

added "certifications" to the config, allowing scoring of a category to be independent of the certification of it. e.g. a site can be ✔PWA but we still give them a bad score cause they're really slow or whatever.

Uses the same styling as before (just a green check).

Wanted to get implementation feedback before adding tests.

paulirish · 2017-05-16T20:19:42Z

lighthouse-core/config/config.js

    });
+
+    // Validate certification (if any).
+    if (category.certification) {


do we want to validate any certification in the config, even if a category doesn't call it out?

do we want to validate any certification in the config

like just check that the audits a certification is requesting will be run? We could do that, but I'm already running into issues with filtering the config with onlyAudits. For this version of validation, I was going to drop the certification property if onlyAudits filtered out a required audit rather than show an error. If we want to validate any certification in the config, I'm not sure what to do in that case...strip out the certification altogether?

i see.

okay then. this works for me.

paulirish · 2017-05-16T20:22:28Z

lighthouse-core/report/v2/report-generator.js

-      pwaCategory.isPWA = isPWA;
-    }
+      // Check certification (if any).
+      let certified;


isCertified

paulirish · 2017-05-16T20:47:48Z

I had envisioned a slight tweak on this, but lmk wyt:

the LHR has a certifications object
within that object is a isCertified boolean
and the reportCategory references that certification by ID.

but no strong feelings. And this approach may be not in-line with how we do report* in the LHR now.

patrickhulce

It seems like the only connection certifications have with categories is we want a particular certification to be displayed in the report as a checkmark on a particular category header, is there a different way to model this so certifications appear more as a standalone thing?

For example it feels weird that we'll be returning a reportCertifications object that does not have a isCertified/passed property and you have to comb through the categories to match it up and find out if it passed.

patrickhulce · 2017-05-16T20:40:30Z

lighthouse-core/config/config.js

    return this._groups;
  }
+
+  /** @type {Object<string, {title: string, description: string}>|undefined} */


this type doesn't look right?

no, that's a c/p mistake

patrickhulce · 2017-05-16T20:41:54Z

lighthouse-core/config/default.js

      "name": "Progressive Web App",
      "weight": 1,
      "description": "These audits validate the aspects of a Progressive Web App, as specified by the baseline [PWA Checklist](https://developers.google.com/web/progressive-web-apps/checklist).",
+      "certification": "is-pwa",


why is it a property of a category? there seems to be no relationship at all to the category shouldn't certifications just be evaluated and returned as their own thing?

why is it a property of a category?

does Paul's idea sound good to you? Top level evaluated certifications, and a category can say, "hey, put a checkmark next to me based on if the certification with this name passed"?

sgtm for now ya 👍

patrickhulce · 2017-05-16T20:42:46Z

lighthouse-core/report/v2/renderer/category-renderer.js

+    const element = this._populateScore(
+        tmpl, score, 'numeric', category.name, category.description);
+
+    if (certDefinitions && category.certification && category.isCertified) {


someone once told me we should trust our closure types and not be defensive beyond them... ;)

but, backwards compat? shrug

haha I agree @paulirish! just wanted to point it out to those who were down on it to give me some slack :P

backwards compat

hehe, no :P

In this case certDefinitions is an optional argument (though arguably perf and a11y categories should get it too), a category might not have a certification, and we only add a check mark if the category is certified...there's no red x for failing certification

perf and a11y categories should get it too

er, perf and a11y category renderers should get it too

patrickhulce · 2017-05-16T20:44:36Z

lighthouse-core/report/v2/report-generator.js

+      if (category.certification) {
+        // Certification and required audits existence checked by Config.
+        const certification = config.certifications[category.certification];
+        isCertified = certification.audits.reduce((result, certAudit) => {


nit: I think Paul is not a reduce fan, could just use .every instead

i'm okay with it here. (as it is reducing down to a single value, rather than pushing items onto an array).

but now that you mention it, every() seems better. :)

every is nice

brendankenny · 2017-05-16T21:30:45Z

ok, so I think I like the idea of structural certifications, where certifications exist independent of categories, and they pass if the audits in question meet a threshold.

Questions:

how to deal with blacklisting things via onlyAudits or onlyCategories? Audits will be removed, and suddenly certifications won't be able to pass?
do we care about failing certifications? One way to fix the above is a LHR either gets a certification or it does't, and "doesn't" may include not meeting minimum scores (failing) or the audit(s) in question never ran
How do we deal with config error checking? Hard fail upon referencing audits that don't exist is ideal, but if we go with the above, we're going to have to allow references to audits that don't exist in the config, which isn't ideal. Easy to miss a misspelled audit name and that a certification just never passes

patrickhulce · 2017-05-16T21:37:23Z

how to deal with blacklisting things via onlyAudits or onlyCategories? Audits will be removed, and suddenly certifications won't be able to pass?

How do we deal with config error checking? Hard fail upon referencing audits that don't exist is ideal, but if we go with the above, we're going to have to allow references to audits that don't exist in the config, which isn't ideal. Easy to miss a misspelled audit name and that a certification just never passes

I'd say we just filter out certifications that can't be met by the result of onlyAudits/onlyCategories when we're doing the filtering stage, that way we still hard fail for misspellings and such but only run certifications when we actually can.

do we care about failing certifications? One way to fix the above is a LHR either gets a certification or it does't, and "doesn't" may include not meeting minimum scores (failing) or the audit(s) in question never ran

IMO we should just provide them in the results JSON for now and special case the is-pwa and future is-exemplary-pwa for display as checkmarks in particular places.

Later on certifications could each appear as nav items to click and visit an overview page for that certification that shows the failures/what comprises the audit/whatever we want

brendankenny · 2017-05-17T03:49:05Z

updated to top level certifications. Like 15 extending corner cases to still test, tho

patrickhulce

I like this a lot, feels much more extensible in the exemplary/distinguished/baller designations to come :)

patrickhulce · 2017-05-17T16:18:57Z

lighthouse-core/report/v2/renderer/category-renderer.js

+      if (certification.isCertified) {
+        const titleEl = element.querySelector('.lh-score__title');
+        const link = /** @type {!Element} */ (titleEl.appendChild(this._dom.createSpanFromMarkdown(
+            `[✔](${certification.url})`)));


should we put an x if it didn't meet the requirements?

patrickhulce · 2017-05-17T16:20:07Z

lighthouse-core/report/v2/renderer/category-renderer.js

+        const titleEl = element.querySelector('.lh-score__title');
+        const link = /** @type {!Element} */ (titleEl.appendChild(this._dom.createSpanFromMarkdown(
+            `[✔](${certification.url})`)));
+        link.classList.add('lh-score__isCertified');


nit: __certification or maybe --certified on lh-score? at the very least __is-certified :)

ebidel · 2017-05-23T18:52:35Z

lighthouse-core/config/config.js

 }

-function validateCategories(categories, audits, auditResults, groups) {
+function validateCategories(categories, audits, auditResults, groups, certifications) {


ebidel · 2017-05-23T18:52:39Z

lighthouse-core/config/config.js

+  });
+}
+
+function validateCertifications(certifications, audits, auditResults) {


ebidel · 2017-05-23T19:07:29Z

lighthouse-core/report/v2/renderer/category-renderer.js

   * @return {!Element}
   */
-  render(category, groups) {
+  render(category, groups, certifications) {


I've been thinking that we should be passing the report object around instead of adding additional params to all of the render methods + piping it throughout. Each method can pull the properties it cares about. WDYT?

paulirish · 2017-07-07T18:17:20Z

Once this boolean (via whitelisting) is in place, then we can move manifest-name-short-enough from Best Practices into PWA

paulirish · 2017-10-12T00:01:50Z

No more demand for this fella it seems.

As of May, a 100 PWA score === this checkmark. We don't see that changing in the short term.

brendankenny · 2017-10-12T00:09:33Z

🎉 🎂

paulirish mentioned this pull request Apr 18, 2017

[meta] I/O Burndown #1937

Closed

52 tasks

ebidel mentioned this pull request Apr 19, 2017

Report PWA results in terms of Initial/Baseline/Showcase definition sets #746

Closed

paulirish reviewed Apr 21, 2017

View reviewed changes

paulirish requested a review from brendankenny April 22, 2017 00:58

brendankenny self-assigned this Apr 25, 2017

paulirish mentioned this pull request May 5, 2017

[meta] I/O The Final Countdown #2161

Closed

40 tasks

brendankenny force-pushed the ispwa branch from c85a193 to 3183813 Compare May 16, 2017 08:15

paulirish added the rls blocker label May 16, 2017

paulirish reviewed May 16, 2017

View reviewed changes

patrickhulce suggested changes May 16, 2017

View reviewed changes

brendankenny force-pushed the ispwa branch from 4e57715 to b7db573 Compare May 17, 2017 03:11

ebidel and others added 4 commits May 16, 2017 20:12

Add isPWA boolean

ff5b94b

add config certifications

0a7637c

s/certified/isCertified

95ddd90

top level certifications

458204c

brendankenny force-pushed the ispwa branch from b7db573 to 458204c Compare May 17, 2017 03:12

restore test

3ea97c5

names not paths

2cf2eae

patrickhulce reviewed May 17, 2017

View reviewed changes

ebidel commented May 23, 2017

View reviewed changes

ebidel added #BOB and removed rls blocker labels Jun 23, 2017

paulirish closed this Oct 12, 2017

paulirish deleted the ispwa branch October 12, 2017 00:01

Conversation

ebidel commented Apr 18, 2017

Uh oh!

patrickhulce commented Apr 18, 2017

Uh oh!

ebidel commented Apr 18, 2017

Uh oh!

ebidel commented Apr 18, 2017

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A wild strawman proposal appears!

Passing isPWA ✓

Failing isPWA ✘

TTI Scoring

Uh oh!

patrickhulce commented Apr 21, 2017

Uh oh!

paulirish commented Apr 22, 2017

Uh oh!

ebidel commented Apr 25, 2017

Uh oh!

brendankenny commented Apr 25, 2017

Uh oh!

ebidel commented May 16, 2017

Uh oh!

brendankenny commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny commented May 16, 2017

Uh oh!

patrickhulce commented May 16, 2017

Uh oh!

brendankenny commented May 17, 2017

paulirish commented Apr 21, 2017 •

edited

Loading

paulirish commented May 16, 2017 •

edited

Loading

patrickhulce May 17, 2017 •

edited

Loading