Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: remove byte-size #3569

Merged
merged 1 commit into from
Aug 11, 2021
Merged

deps: remove byte-size #3569

merged 1 commit into from
Aug 11, 2021

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.

lib/view.js Outdated Show resolved Hide resolved
@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.

@@ -28,8 +28,8 @@ const logTar = (tarball, opts = {}) => {
{ name: 'name:', value: tarball.name },
{ name: 'version:', value: tarball.version },
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