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

rfc: install size information logging #1

Closed
wants to merge 1 commit into from

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Apr 26, 2018

No description provided.

#### `npm install --json`

The JSON format will add two things: `size` and `unpackedSize` field to each
individual `action` record, and a `sizeDelta` field that totals up all the
Copy link

Choose a reason for hiding this comment

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

Should sizeDelta be unpackedSizeDelta? It might be misleading since size and sizeDelta are currently packed vs unpacked respectively.

Choose a reason for hiding this comment

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

I think so :/! The main focus is on the size added to the node_modules folder right?
OBS.: Just let me left my congrats, specially to @zkat to keep this feature so aimed to the real focus!! Really great job!


There are, of course, other alternatives (which can still be considered):

* Have the npm website itself calculate and host this information (initially without project-level awareness, but we could extend it by integrating more with projects or using uploaded metadata from newer npm versions). This would be a similar approach to [packagephoia.now.sh](https://packagephobia.now.sh).
Copy link

Choose a reason for hiding this comment

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

Typo here. packagephoia.now.sh should be packagephobia.now.sh


* Have the npm website itself calculate and host this information (initially without project-level awareness, but we could extend it by integrating more with projects or using uploaded metadata from newer npm versions). This would be a similar approach to [packagephoia.now.sh](https://packagephobia.now.sh).
* Create a separate command to do this analysis. If we do this _instead_ of the current proposal, we'd lose the "default information" part, and rely on discoverability to show this information to users. I think this would make the feature much less useful.
* The biggest drawback of this proposal, though, in my opinion, is that it does not specifically give _bundle size information_. While I don't think there's any way to come up with exact bundle size information without running the specific bundler the user is using, with the user's specific information, on every single install, and somehow doing a size diff based on that, there may be a compromise to be had: npm might be able to detect whether a bundler is being _used_, and then spit out a pre-calculated bundle estimate (with minification and gzip).
Copy link

Choose a reason for hiding this comment

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

It might be worth noting that https://bundlephobia.com can provide this bundle size.
/cc @pastelsky


* [packagephobia.now.sh](https://packagephobia.now.sh/) -- service-based, allows pretty fast searches, but gives numbers centered more around node-side dependencies than bundle sizes -- so getting an idea about compression is impossible, and furthermore, the sizes are wildly inaccurate for a particular project because it doesn't take into account per-project deduplication. It only calculates this flattening at the level of the dependency itself. I do not believe this style of tool serves our purposes.
* [`yarn why`](https://yarnpkg.com/en/docs/cli/why) -- this does give some level of this information, but it's not quite prepared in a way that's useful for estimating the specific impact of the package on a bundle. Furthermore, the command seems fairly broken and inaccurate as of 1.6.0 (???), so there might be something strange about how it calculates things. A nice thing about this command is that it can be used after installation to start hunting down disk-hungry packages, which is a workflow this proposal does not account for.

Copy link

Choose a reason for hiding this comment

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

It might be worth noting that cost-of-modules provides a breakdown of installed package sizes. Although the focus is primarily on your already installed packages.

/cc @siddharthkp

@johana-star
Copy link

👀

Copy link

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I left a couple comments: Possibly, a couple of typos

@zkat zkat force-pushed the zkat/rfc-install-size-info branch from d5e492b to 073f34c Compare April 27, 2018 22:20
Copy link

@styfle styfle left a comment

Choose a reason for hiding this comment

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

@zkat There are a couple questions in this document. I think I approve the structure, but do you except to resolve those questions before merging? If the questions are for after the merge, then I approve 🙂

@johana-star
Copy link

johana-star commented May 1, 2018

I haven't had as many spare cycles as I expected. I've made preliminary steps (cloned down the code, ran the specs, reviewed glanced at the code which will need changing) but haven't gotten traction enough for an initial code change yet. When I do have work in progress, I'll throw a PR against npm/npm referencing this RFC.

Does closing the RFC PR mean that the proposal is formally ready to be worked on?

@zkat
Copy link
Contributor Author

zkat commented May 2, 2018

@strand this RFC is considered "ratified" after we merge it, correct. Until then, a lot of stuff is up in the air.

I had some conversations with the registry folks, and it's possible that in the next couple of months we'll backfill tarball information using do-you-even-lift, but until then, we won't be able to actually execute this RFC properly. You're free to start poking at an implementation, and that sort of thing can help get more ideas, but this RFC is at a pretty early stage right now so it's bound to change a lot.

@johana-star
Copy link

I'm glad I've gotten some familiarity with the codebase and a high-level understanding of the problem, and will let this work lay fallow until the ratification and tooling work is completed.

@zkat zkat requested a review from ceejbot May 9, 2018 21:42
@zkat
Copy link
Contributor Author

zkat commented May 9, 2018

TODO: I need to update this with the intended/expected additions to the registry, as I'm thinking of them now, and how those are going to be put together into a final answer, and we can iterate over it once registry folks can take a gander.

@zkat
Copy link
Contributor Author

zkat commented Jan 30, 2019

Brief update: We've moved this internally to appropriate channels and we'll be doing an internal review + prioritization of this feature. No timeline until that's done.

@settings settings bot removed the registry label Oct 30, 2019
@darcyclarke darcyclarke added Enhancement new feature or improvement Needs Discussion is pending a discussion semver:minor new backwards-compatible feature labels Oct 30, 2019
@darcyclarke darcyclarke removed request for a team and ceejbot October 30, 2019 04:49
@zkat zkat closed this Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement Needs Discussion is pending a discussion semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants