Skip to content

Conversation

@wraithgar
Copy link
Member

@wraithgar wraithgar commented Jul 22, 2021

In its latest release, byte-size dropped support for node versions lower
than 14. The cli currently supports node 10 and up.

The actual functionality we needed and was using was extremely limited
in scope, and didn't warrant an external module. It's just pretty
printing a file size, and the files we are dealing with are limited in
size so we don't need to support so many suffixes.

@wraithgar wraithgar requested a review from a team as a code owner July 22, 2021 14:55
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 7.x work is associated with a specific npm 7 release labels Jul 22, 2021
@wraithgar
Copy link
Member Author

NOTE even pretty-bytes will soon not work in node10 so we may need to just implement this ourselves: sindresorhus/pretty-bytes#68

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2021

Seems like it might be better to just stay on v7 of byte-size in the meantime.

@wraithgar
Copy link
Member Author

Seems like it might be better to just stay on v7 of byte-size in the meantime.

The thing we're balancing this against is having outdated packages in the tree, we already have two that have pending reasons why they can't be updated at this time and every time the list is added to it becomes harder to manage. We're trying to be pretty aggressive on this front.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2021

Understood, but it seems like adopting a package that is explicitly planning to be nonviable within the next 5 months is just hiding/punting the tech debt and difficulty, not reducing it.

@wraithgar
Copy link
Member Author

Yes this is absolutely a calculated tech debt punting decision.

@wraithgar wraithgar marked this pull request as draft July 22, 2021 17:53
@darcyclarke darcyclarke added this to the OSS - Sprint 35 milestone Aug 9, 2021
@wraithgar wraithgar changed the title deps: move from byte-size to pretty-bytes deps: remove byte-size Aug 9, 2021
@wraithgar wraithgar marked this pull request as ready for review August 9, 2021 21:37
@wraithgar
Copy link
Member Author

Rewrote this with @fritzy's help to just be the absolute minimum functionality we need, we're not using an entire formatting library here we're just translating a very small section of file size values.

tarball.filename && { name: 'filename:', value: tarball.filename },
{ name: 'package size:', value: byteSize(tarball.size) },
{ name: 'unpacked size:', value: byteSize(tarball.unpackedSize) },
{ name: 'package size:', value: formatBytes(tarball.size) },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are actually used anywhere because byteSize would be returning an object here not a string.

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

In its latest release, byte-size dropped support for node versions lower
than 14.  The cli currently supports node 10 and up.

The actual functionality we needed and was using was extremely limited
in scope, and didn't warrant an external module.  It's just pretty
printing a file size, and the files we are dealing with are limited in
size so we don't need to support so many suffixes.

PR-URL: #3569
Credit: @wraithgar
Close: #3569
Reviewed-by: @isaacs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants