Add ability to format bytes as metric or IEC; affects [bundlejs bundlephobia CratesSize DockerSize GithubRepoSize GithubCodeSize GithubSize NpmUnpackedSize SpigetDownloadSize steam VisualStudioAppCenterReleasesSize whatpulse] #10547
Conversation
- 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
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@actions/[email protected], npm/@actions/[email protected] |
| * @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') { |
There was a problem hiding this comment.
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.
| const size = json.size.compressedSize | ||
| return this.constructor.render({ size }) | ||
| const size = json.size.rawCompressedSize | ||
| return renderSizeBadge(size, 'metric', 'minified size (gzip)') |
There was a problem hiding this comment.
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
| 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') |
There was a problem hiding this comment.
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
| 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') |
There was a problem hiding this comment.
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.
| async handle({ owner, app, token }) { | ||
| const { size } = await this.fetch({ owner, app, token, schema }) | ||
| return this.constructor.render({ size }) | ||
| return renderSizeBadge(size, 'metric') |
There was a problem hiding this comment.
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.
| .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' }) |
There was a problem hiding this comment.
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.
|
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
changed the title
| const queryParamSchema = baseQueryParamSchema.keys({ | ||
| units: unitsQueryParam.default(defaultUnits), | ||
| }) |
There was a problem hiding this comment.
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())
|
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. |
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.