core(lhr): support printf displayValues #5099

patrickhulce · 2018-05-03T17:13:16Z

adds support for array-based display values in a printf style combined with our granularity for formatting numbers, I converted the display values that were most likely to be affected by this (numbers that can reach 1000+), everything else doesn't necessarily need to change but the breaking change has been made and we're free to move between them now :)

I wanted to do the conversion in the report at the beginning so everything else can assume it's a string, but there were only 3 callers, so it didn't seem like a big deal. LMK if others prefer that approach

ref #5008 #4333

paulirish · 2018-05-03T20:32:53Z

lighthouse-core/test/report/html/renderer/util-test.js

+    const format = (...args) => Util.formatDisplayValue(...args);
+    assert.equal(format(undefined), '');
+    assert.equal(format([1]), 'UNKNOWN');
+    assert.equal(format(['%s %s', 'Hello', 'Paul']), 'Hello Paul');


Hello formatDisplayValue

paulirish · 2018-05-03T20:47:54Z

lighthouse-core/report/html/renderer/util.js

+
+    let output = template;
+    while (displayValue.length) {
+      const replacement = /** @type {number|string} */ (displayValue.shift());


assert.equal(format(['%s']), '%s', 'not enough replacements'); assert.equal(format(['%s', 'a', 'a']), 'a', 'extra replacements');

i think we may want to console.warn() in these cases. Allow them, but raise a warning flag.

paulirish · 2018-05-03T20:54:37Z

lighthouse-core/report/html/renderer/util.js

+      const replacement = /** @type {number|string} */ (displayValue.shift());
+      output = output.replace(/%([0-9.]+)?(d|s)/, match => {
+        const granularity = Number(match.match(/[0-9.]+/)) || 1;
+        return match === '%s' ?


assert.equal(format(['%10s', 123.456]), '120');

currently this does pass, fwiw. shoudl we also throw a warning if you are matching 's' and have a granularity?

modified regex to not match this 👍

paulirish · 2018-05-03T20:56:40Z

lighthouse-core/report/html/renderer/util.js

+   */
+  static formatDisplayValue(displayValue) {
+    if (typeof displayValue === 'string' || typeof displayValue === 'undefined') {
+      return displayValue || '';


i'd prefer being super clear

if (typeof displayValue === 'string') return displayValue; if (typeof displayValue === 'undefined') return '';

brendankenny · 2018-05-03T20:54:05Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

    let displayValue = result.displayValue || '';
    if (typeof result.displayValue === 'undefined' && wastedBytes) {
-      displayValue = `Potential savings of ${wastedBytes} bytes`;
+      displayValue = ['Potential savings of %d\xa0KB', wastedKb];


nit: switch conditional above to testing wastedKb

brendankenny · 2018-05-03T21:07:18Z

lighthouse-core/test/audits/estimated-input-latency-test.js

@@ -29,7 +30,7 @@ describe('Performance: estimated-input-latency audit', () => {
    return Audit.audit(artifacts, {options, settings}).then(output => {
      assert.equal(output.debugString, undefined);
      assert.equal(Math.round(output.rawValue * 10) / 10, 17.1);
-      assert.equal(output.displayValue, '17\xa0ms');
+      assert.equal(Util.formatDisplayValue(output.displayValue), '17\xa0ms');


test the real object here like dom-size-test.js above?

brendankenny · 2018-05-03T21:07:33Z

lighthouse-core/test/audits/first-meaningful-paint-test.js

@@ -26,7 +27,7 @@ describe('Performance: first-meaningful-paint audit', () => {
    const fmpResult = await FMPAudit.audit(artifacts, context);

    assert.equal(fmpResult.score, 1);
-    assert.equal(fmpResult.displayValue, '780\xa0ms');
+    assert.equal(Util.formatDisplayValue(fmpResult.displayValue), '780\xa0ms');


same with these

brendankenny · 2018-05-03T21:07:41Z

lighthouse-core/test/audits/interactive-test.js

@@ -33,7 +34,7 @@ describe('Performance: interactive audit', () => {
    return Interactive.audit(artifacts, {options, settings}).then(output => {
      assert.equal(output.score, 1);
      assert.equal(Math.round(output.rawValue), 1582);
-      assert.equal(output.displayValue, '1,580\xa0ms');
+      assert.equal(Util.formatDisplayValue(output.displayValue), '1,580\xa0ms');


and these :)

brendankenny · 2018-05-03T21:09:23Z

typings/audit.d.ts

@@ -23,6 +23,8 @@ declare global {

    export type ScoringModeValue = Audit.ScoringModes[keyof Audit.ScoringModes];

+    export type DisplayValue = string | Array<string|number>


there might be an even more elegant way to do this, but you can do

interface DisplayValueArray extends Array<string | number> { 0: string; } export type DisplayValue = string | DisplayValueArray;

to enforce a string in the first position (and a length of at least 1)

brendankenny · 2018-05-03T21:12:21Z

lighthouse-core/test/report/html/renderer/util-test.js

@@ -53,4 +53,16 @@ describe('util helpers', () => {
    assert.equal(Util.calculateRating(0.80), 'pass');
    assert.equal(Util.calculateRating(1.00), 'pass');
  });
+
+  it('formats display values', () => {
+    const format = (...args) => Util.formatDisplayValue(...args);


don't need to rest and then spread since they're all arrays below?

brendankenny · 2018-05-03T21:16:10Z

lighthouse-core/report/html/renderer/util.js

+
+    let output = template;
+    while (displayValue.length) {
+      const replacement = /** @type {number|string} */ (displayValue.shift());


just switch to regular indexing and avoid these shift assertions? Or could be a reduce

brendankenny · 2018-05-03T21:17:36Z

lighthouse-core/report/html/renderer/util.js

+    let output = template;
+    while (displayValue.length) {
+      const replacement = /** @type {number|string} */ (displayValue.shift());
+      output = output.replace(/%([0-9.]+)?(d|s)/, match => {


this allows more than one decimal point? and no support for granularity when it's a string?

correct if you mess up your format string it'll put NaN
correct %s is just straight up replacement

I meant should it be more like /%([0-9]*(?:\.[0-9]*))?d|s/ or whatever (you covered the no granularity + string case now)

sure, tweaked it a bit

brendankenny · 2018-05-03T22:51:04Z

lighthouse-core/test/audits/first-meaningful-paint-test.js

@@ -39,7 +40,7 @@ describe('Performance: first-meaningful-paint audit', () => {
    const fmpResult = await FMPAudit.audit(artifacts, context);

    assert.equal(fmpResult.score, 0.79);
-    assert.equal(fmpResult.displayValue, '2,850\xa0ms');
+    assert.equal(Math.round(fmpResult.displayValue[1]), 2851);


lol I meant the whole display value to ensure the whole thing matched what's expected (including format string) but can also revert to the last commit for these if you'd prefer that :)

will do (revert that is) :)

brendankenny

LGTM!

patrickhulce added 3 commits May 3, 2018 09:12

core(displayValue): display value as array

6e8ae64

tests

dc4fc72

test updates

bbb8c35

patrickhulce requested review from brendankenny, paulirish and vinamratasingal-zz as code owners May 3, 2018 17:13

nbsp

7af9dd0

paulirish mentioned this pull request May 3, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

paulirish requested changes May 3, 2018

View reviewed changes

brendankenny requested changes May 3, 2018

View reviewed changes

patrickhulce added 2 commits May 3, 2018 14:34

feedback

8208002

Merge branch 'master' into displayvalue

37fbd53

paulirish approved these changes May 3, 2018

View reviewed changes

brendankenny reviewed May 3, 2018

View reviewed changes

patrickhulce added 2 commits May 3, 2018 16:06

better regex

0abca9b

Merge branch 'master' into displayvalue

790158b

brendankenny approved these changes May 3, 2018

View reviewed changes

Merge branch 'master' into displayvalue

2d69e89

paulirish changed the title ~~core(displayValue): support printf displayValues~~ core(lhr): support printf displayValues May 3, 2018

GoogleChrome deleted a comment from googlebot May 3, 2018

remove fanciness

141103d

GoogleChrome deleted a comment from googlebot May 3, 2018

brendankenny merged commit 91984d7 into master May 4, 2018

brendankenny deleted the displayvalue branch May 4, 2018 00:05

brendankenny mentioned this pull request May 4, 2018

core(tsc): fancier displayValue type #5111

Merged

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core(lhr): support printf displayValues (GoogleChrome#5099)

785727e

brendankenny mentioned this pull request Aug 5, 2019

misc: share markdown parsing in collect-strings and the report #9514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lhr): support printf displayValues #5099

core(lhr): support printf displayValues #5099

patrickhulce commented May 3, 2018 •

edited

Loading

paulirish May 3, 2018

patrickhulce May 3, 2018

paulirish May 3, 2018

patrickhulce May 3, 2018

paulirish May 3, 2018

patrickhulce May 3, 2018

paulirish May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

brendankenny May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny May 3, 2018

patrickhulce May 3, 2018

brendankenny left a comment

		@@ -23,6 +23,8 @@ declare global {

		export type ScoringModeValue = Audit.ScoringModes[keyof Audit.ScoringModes];

		export type DisplayValue = string \| Array<string\|number>

core(lhr): support printf displayValues #5099

core(lhr): support printf displayValues #5099

Conversation

patrickhulce commented May 3, 2018 • 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

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

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

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

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

patrickhulce commented May 3, 2018 •

edited

Loading