i18n: remove default granularity values from formatter #13839

connorjclark · 2022-04-08T20:46:52Z

We had various default values for granularity in the i18n formatter module. For formatNumber particularly, this made that method less usable outside details rendering, where these default granularities values were intended, and the assumption that "round to the nearest tenth" didn't hold for the one use case outside details rendering.

this pr changes the default behavior of these formatting method to do no granularity/rounding (asterisk, except what is defined in the Intl api), and moves the default granularity values to the details renderer.

Passing no granularity is equivalent to using 0 for minimumFractionDigits and 3 for maximumFractionDigits, which is behavior that cannot be replicated with our granularity abstraction. And makes for a more usable default behavior.

There should be zero changes to report rendering, besides getEmulationDescriptions.

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

adamraine · 2022-04-11T18:05:17Z

report/renderer/i18n.js

-    if (Object.is(number, -0)) number = 0;
+      // Avoid displaying a negative value that rounds to zero as "0".
+      if (Object.is(number, -0)) number = 0;
+    } else if (Math.abs(number) <= 0.0001) {


Can you add a comment explaining where 0.0001 comes from.

I think it also needs to be Math.abs(number) < 0.0005 (less than half the smallest expressible value).

e.g. with this _formatNumberWithGranularity(-0.0004) currently gives '-0'

report/test/renderer/i18n-test.js

adamraine · 2022-04-11T18:09:14Z

docs/new-audits.md

+- `ms`
+- `numeric`
+
+`granularity` must either be a positive integer or power of 10. Some examples of valid values for `granularity`:


When you say "power of 10" you mean "integer power of 10" right? If so I don't think 0.5 qualifies.

updated but... maybe we could just reduce the restriction on granularity to any positive number?

report/renderer/details-renderer.js

brendankenny · 2022-04-11T18:34:11Z

report/renderer/i18n.js

-    if (Object.is(number, -0)) number = 0;
+      // Avoid displaying a negative value that rounds to zero as "0".
+      if (Object.is(number, -0)) number = 0;
+    } else if (Math.abs(number) <= 0.0001) {


I think it also needs to be Math.abs(number) < 0.0005 (less than half the smallest expressible value).

e.g. with this _formatNumberWithGranularity(-0.0004) currently gives '-0'

brendankenny · 2022-04-11T18:35:42Z

report/renderer/i18n.js

-    if (!Number.isFinite(log10) || (granularity > 1 && Math.floor(log10) !== log10)) {
-      throw new Error(`granularity of ${granularity} is invalid`);
-    }
+    if (granularity !== undefined) {


it does feel like we're working too hard to maintain granularity which was kind of a haphazard choice long ago, but it's unfortunately in the LHR, so...:(

adamraine · 2022-04-11T20:33:58Z

report/renderer/i18n.js

@@ -32,36 +32,42 @@ export class I18n {

  /**
   * @param {number} number
-   * @param {number} granularity
+   * @param {number|undefined} granularity
   * @param {Intl.NumberFormatOptions} opts


nit: seems like we can make granularity and opts optional. opts even has a default value.

Bump on this if it's not too much work.

just now: I added = to the opts type to better reflect how this method is used.

granularity should remain undefined because all the use cases here are passing a value explicitly, even if it may be undefined.

all this stuff is pretty subjective i think. if you feel strongly I'll make the change.

Yeah that's fine with me

adamraine · 2022-04-11T20:34:54Z

report/renderer/i18n.js

   * @return {string}
   */
-  formatNumber(number, granularity = 0.1) {
+  formatNumber(number, granularity = undefined) {


nit: optional params are already undefined by default

Even so, I think having a second way to represent "this is optional" is nice.

I'm definitely for not including = undefined for optional params

Yeah I don't think we do this anywhere else. I still prefer without = undefined

report/renderer/i18n.js

docs/new-audits.md

adamraine

I have a couple outstanding nits but otherwise LGTM

adamraine · 2022-04-26T16:28:47Z

report/renderer/i18n.js

@@ -32,36 +32,42 @@ export class I18n {

  /**
   * @param {number} number
-   * @param {number} granularity
+   * @param {number|undefined} granularity
   * @param {Intl.NumberFormatOptions} opts


Bump on this if it's not too much work.

connorjclark and others added 13 commits April 6, 2022 15:15

first pass, localize units

504e62a

localize formatDuration

0b88142

hmm

7dd7d01

update

08bc98b

Update lighthouse-core/audits/bootup-time.js

b4a8e23

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

pr

61c23f5

kb/s

6232d93

update

9e82306

fix

325d84b

fix

0d3cbc6

formatNumber gran default

8b0fa70

first pass removing default granularity from formatter functions

bf94f1d

i18n: remove default granularity values from formatter

125a7bd

connorjclark requested a review from a team as a code owner April 8, 2022 20:46

connorjclark requested review from adamraine and removed request for a team April 8, 2022 20:46

devtools-bot assigned adamraine Apr 8, 2022

devtools-bot added the waiting4reviewer label Apr 8, 2022

Merge remote-tracking branch 'origin/master' into i18n-formatter-params

929ba50

vercel bot deployed to Preview April 11, 2022 18:07 View deployment

adamraine reviewed Apr 11, 2022

View reviewed changes

brendankenny reviewed Apr 11, 2022

View reviewed changes

update

893968b

vercel bot deployed to Preview April 11, 2022 20:09 View deployment

tests

f11e9f0

vercel bot deployed to Preview April 11, 2022 20:11 View deployment

adamraine reviewed Apr 11, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into i18n-formatter-params

1e800ba

vercel bot deployed to Preview April 19, 2022 18:21 View deployment

Base automatically changed from i18n-formatter-units to master April 19, 2022 20:58

update

d33c2d0

vercel bot deployed to Preview April 19, 2022 21:16 View deployment

connorjclark commented Apr 19, 2022

View reviewed changes

docs/new-audits.md Show resolved Hide resolved

connorjclark added 2 commits April 25, 2022 13:30

update

803fcc7

fix

ded8d2c

vercel bot deployed to Preview April 25, 2022 20:33 View deployment

oops

c064061

vercel bot deployed to Preview April 26, 2022 00:43 View deployment

adamraine approved these changes Apr 26, 2022

View reviewed changes

change

749c705

vercel bot deployed to Preview April 26, 2022 19:26 View deployment

Merge remote-tracking branch 'origin/master' into i18n-formatter-params

c04d995

vercel bot deployed to Preview April 27, 2022 17:57 View deployment

update

55b9a52

vercel bot deployed to Preview April 27, 2022 18:37 View deployment

connorjclark merged commit 83121be into master Apr 27, 2022

connorjclark deleted the i18n-formatter-params branch April 27, 2022 20:49

connorjclark mentioned this pull request Jul 14, 2023

core(duplicated-javascript): use valid granularity #15275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: remove default granularity values from formatter #13839

i18n: remove default granularity values from formatter #13839

connorjclark commented Apr 8, 2022

adamraine Apr 11, 2022

brendankenny Apr 11, 2022

adamraine Apr 11, 2022

connorjclark Apr 11, 2022

brendankenny Apr 11, 2022

brendankenny Apr 11, 2022

adamraine Apr 11, 2022

adamraine Apr 26, 2022

connorjclark Apr 26, 2022

adamraine Apr 26, 2022

adamraine Apr 11, 2022

connorjclark Apr 26, 2022

brendankenny Apr 26, 2022

adamraine Apr 26, 2022

adamraine left a comment

adamraine Apr 26, 2022

i18n: remove default granularity values from formatter #13839

i18n: remove default granularity values from formatter #13839

Conversation

connorjclark commented Apr 8, 2022

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

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment