Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia ChromeWebStoreSize CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547

chris48s · 2024-09-21T18:08:34Z

Following on from the conversation in #10437

Closes #10437
Closes #9703
Refs #8845
Refs #10421

While this PR does introduce some variance, I have also taken the opportunity to add a renderSizeBadge() helper function which will help us achieve some more horizontal consistency around the things where we do want to be opinionated.

- switch from pretty-bytes to byte-size - add renderSizeBadge() helper function - match upstream conventions for metric/IEC units - add new test helpers and use them in service tests

socket-security · 2024-09-21T18:09:34Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@actions/[email protected]	Transitive: environment, filesystem, network, shell, unsafe	`+6`	1.56 MB	bryanmacfarlane, chrispat, cschleiden, ...2 more
npm/@actions/[email protected]	environment, filesystem Transitive: network, unsafe	`+20`	8.09 MB	thboop
npm/[email protected]	None	`0`	32.3 kB	75lb

🚮 Removed packages: npm/[email protected]

View full report↗︎

github-actions · 2024-09-21T18:09:48Z

	Messages
📖	✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 7a13ca8

chris48s · 2024-09-21T18:11:27Z

services/size.js

+ * @param {string} [label='size'] - Custom label
+ * @returns {object} A badge object that has three properties: label, message, and color
+ */
+function renderSizeBadge(bytes, units, label = 'size') {


This helper gives us

a standard colour

a default label text (although we can override it) and

always passes the number of bytes though byteSize() with the same settings

byte-size takes a slightly different approach to rounding than pretty-bytes does.
In pretty-bytes, rounding level is expressed as significant figures. The default in pretty-bytes was 3 significant figures.
In byte-size, rounding level is expressed as decimal places. The default in byte-size is 1 decimal place.
For some examples, this will give us a slightly different level of rounding
My instinct is to roll with the the default (1 decimal place), but 2 decimal places would also be a reasonable choice here. Either way, lets make a decision now, set it here in the renderSizeBadge() helper, and use that everywhere.

chris48s · 2024-09-21T18:12:09Z

services/bundlejs/bundlejs-package.service.js

-    const size = json.size.compressedSize
-    return this.constructor.render({ size })
+    const size = json.size.rawCompressedSize
+    return renderSizeBadge(size, 'metric', 'minified size (gzip)')


Previously we were presenting compressedSize as reported by the upstream
Switching this to use rawCompressedSize allows us to apply our own rounding convention while matching the upstream units

chris48s · 2024-09-21T18:12:55Z

services/github/github-repo-size.service.js

-    return this.constructor.render({ size })
+    // note the GH API returns size in KiB
+    // so we multiply by 1024 to get a size in bytes and then format that in IEC bytes
+    return renderSizeBadge(size * 1024, 'iec', 'repo size')


The GitHub API exposes a number of Kibibytes (although the API docs say "kilobytes") rather than a number of bytes.

There is one place in the GitHub UI where this number is exposed/formatted and that is on https://github.com/settings/repositories

To work an example, if I call https://api.github.com/repos/chris48s/geometry-to-spatialite I get size=7159. That's presented in the frontend as

7159/1024 = 6.99

chris48s · 2024-09-21T18:13:15Z

services/github/github-code-size.service.js

  async handle({ user, repo }) {
    const data = await this.fetch({ user, repo })
-    return this.constructor.render({ size: this.getTotalSize(data) })
+    return renderSizeBadge(this.getTotalSize(data), 'iec', 'code size')


This number is never presented anywhere in the GitHub UI. Since the other 2 GitHub badges use IEC bytes, lets go with that here too.

chris48s · 2024-09-21T18:13:31Z

services/visual-studio-app-center/visual-studio-app-center-releases-size.service.js

  async handle({ owner, app, token }) {
    const { size } = await this.fetch({ owner, app, token, schema })
-    return this.constructor.render({ size })
+    return renderSizeBadge(size, 'metric')


I have no idea how this site presents sizes. Lets stick with metric for the moment on the basis we have been doing that for some time and nobody has complained yet. We can change it if we need to.

chris48s · 2024-09-21T18:14:03Z

services/npm/npm-unpacked-size.tester.js

-  .expectBadge({ label: 'unpacked size', message: '147 kB' })
+  .expectBadge({ label: 'unpacked size', message: '147.2 kB' })

 t.create('Unpacked size for scoped package')
  .get('/@testing-library/react.json')
-  .expectBadge({ label: 'unpacked size', message: isFileSize })
+  .expectBadge({ label: 'unpacked size', message: isMetricFileSize })

 t.create('Unpacked size for scoped package with version')
  .get('/@testing-library/react/14.2.1.json')
-  .expectBadge({ label: 'unpacked size', message: '5.41 MB' })
+  .expectBadge({ label: 'unpacked size', message: '5.4 MB' })


This file is a good example of the difference between rounding to significant figures vs rounding to decimal places. Switching to 1 decimal place sometimes gives us more precision and sometimes less precision than 3 significant figures.

chris48s · 2024-09-21T18:14:52Z

This PR is still draft as I have not yet added the ability to switch between units via a URL param. I will do that next before marking as ready for review.

not strictly related to this PR but I noticed it was broken

chris48s · 2024-09-30T12:59:58Z

services/npm/npm-unpacked-size.service.js

+const queryParamSchema = baseQueryParamSchema.keys({
+  units: unitsQueryParam.default(defaultUnits),
+})


Here's a little Joi TIL

import Joi from 'joi' const schema1 = Joi.object({ one: Joi.string(), }).required() console.log(schema1.describe()) const schema2 = schema1.keys({ two: Joi.string(), }) console.log(schema2.describe())

chris48s · 2024-09-30T13:06:52Z

OK. I've marked this PR ready for review. In c6028f1 I've added the ability to switch between IEC and metric units. The code isn't bad but it does add a bunch of complexity. Now that I've written it and we have a diff to discuss, I think its reasonable to not only ask "is this good code?" but also to revisit "is this a good idea?"

Even though I've spent a bunch of time on c6028f1 I'd also be quite happy to roll it back and just merge 3395601 and cbac43b if we don't think it is worth it.

PyvesB · 2024-10-06T15:21:43Z

OK. I've marked this PR ready for review. In c6028f1 I've added the ability to switch between IEC and metric units. The code isn't bad but it does add a bunch of complexity. Now that I've written it and we have a diff to discuss, I think its reasonable to not only ask "is this good code?" but also to revisit "is this a good idea?"

I'm a little short on time to do a full review, but the diff with the parameter switch feels reasonable in terms of added code complexity, i.e. at a glance I'm able to follow what's going on.

Regarding whether it's a good idea, honestly, I don't know:

in my mind, I've always associated kilobytes to 1024 bytes. If I see kB or kiB, they're the same thing for me, I'll admit that I never encountered the word "kibibyte" before Add ability to format bytes as metric (e.g: kilobyte) or IEC (e.g: kibibyte) format #10437, or at least it didn't attract my attention to the point that I'd remember. Possible consequence of having a formal education where we often reasoned using the binary system/powers of two, and having never worked on a professional problem when this subtlety mattered enough.
consequently, I'll echo Add ability to format bytes as metric (e.g: kilobyte) or IEC (e.g: kibibyte) format #10437 (comment). Whilst I agree with the general conclusion of vertical consistency, I fear that allowing the same badge (or at least that look the same, assuming I'm not the only idiot to not be able to tell at a glance the difference between kB and kiB) have different meanings depending on a metric parameter, may do more harm than good. I kind of feel like not having the parameter brings the best of both worlds, vertical consistency (same as the number indicated by e.g. upstream bundlephobia service) and horizontal consistency within the service (all e.g. bundlephobia badges show numbers representing the same thing).

Those are my two cents, but ultimately, I don't feel that strongly.

github-actions · 2024-10-16T16:47:55Z

🚀 Updated review app: https://pr-10547-badges-shields.fly.dev

chris48s · 2024-10-21T14:23:57Z

TODO: ~~need to update this after #10613 was merged~~

This reverts commit c6028f1.

and resolve conflicts

chris48s · 2024-11-24T16:02:33Z

I dropped the ball on this one for a while, but coming back to it..

I think I agree with the assessment that for a lot of users kB and KiB is a really subtle distinction and the metric param is probably not useful. I've rolled that change back in 12c6823 .

I've merged in master and resolved the conflicts.

I've updated the PR to account for the Chrome Web Store Size badge, which was added since I opened this PR.

There are a few failing service tests unrelated to the changes in this PR:

There is a PR open for the crates.io failure at remove obsolete [CratesSize] test for null size #10688
The bundlephobia failures are probably transient. We seem to be getting 502s from them at the moment

add renderSizeBadge helper, use it everywhere

3395601

- switch from pretty-bytes to byte-size - add renderSizeBadge() helper function - match upstream conventions for metric/IEC units - add new test helpers and use them in service tests

chris48s added the core Server, BaseService, GitHub auth, Shared helpers label Sep 21, 2024

chris48s commented Sep 21, 2024

View reviewed changes

chris48s added 2 commits September 30, 2024 13:09

unrelated: fix npm unpacked size query param schema

cbac43b

not strictly related to this PR but I noticed it was broken

add param for switching between metric/IEC

c6028f1

chris48s commented Sep 30, 2024

View reviewed changes

Merge branch 'master' into 10437-bytes

4513955

chris48s marked this pull request as ready for review September 30, 2024 13:05

chris48s added 3 commits November 24, 2024 15:29

Revert "add param for switching between metric/IEC"

12c6823

This reverts commit c6028f1.

Merge branch 'master' into 10437-bytes

2e64aca

and resolve conflicts

chromewebstore: reformat size string, test against isIecFileSize

7a13ca8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia ChromeWebStoreSize CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547

Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia ChromeWebStoreSize CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547

chris48s commented Sep 21, 2024

socket-security bot commented Sep 21, 2024 •

edited

Loading

github-actions bot commented Sep 21, 2024 •

edited

Loading

chris48s Sep 21, 2024 •

edited

Loading

chris48s Sep 21, 2024

chris48s Sep 21, 2024 •

edited

Loading

chris48s Sep 21, 2024

chris48s Sep 21, 2024

chris48s Sep 21, 2024

chris48s commented Sep 21, 2024

chris48s Sep 30, 2024

chris48s commented Sep 30, 2024

PyvesB commented Oct 6, 2024

github-actions bot commented Oct 16, 2024

chris48s commented Oct 21, 2024 •

edited

Loading

chris48s commented Nov 24, 2024

Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia ChromeWebStoreSize CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547

Are you sure you want to change the base?

Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia ChromeWebStoreSize CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547

Conversation

chris48s commented Sep 21, 2024

socket-security bot commented Sep 21, 2024 • edited Loading

github-actions bot commented Sep 21, 2024 • edited Loading

chris48s Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

chris48s Sep 21, 2024

Choose a reason for hiding this comment

chris48s Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

chris48s Sep 21, 2024

Choose a reason for hiding this comment

chris48s Sep 21, 2024

Choose a reason for hiding this comment

chris48s Sep 21, 2024

Choose a reason for hiding this comment

chris48s commented Sep 21, 2024

chris48s Sep 30, 2024

Choose a reason for hiding this comment

chris48s commented Sep 30, 2024

PyvesB commented Oct 6, 2024

github-actions bot commented Oct 16, 2024

chris48s commented Oct 21, 2024 • edited Loading

chris48s commented Nov 24, 2024

socket-security bot commented Sep 21, 2024 •

edited

Loading

github-actions bot commented Sep 21, 2024 •

edited

Loading

chris48s Sep 21, 2024 •

edited

Loading

chris48s Sep 21, 2024 •

edited

Loading

chris48s commented Oct 21, 2024 •

edited

Loading