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

Add install size badge to README #3710

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Aug 9, 2018

This adds a new badge to the top of the README.md file to display the install size for express and it's dependencies.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

IDK about adding this badge, though, but at a minimum it needs to be served up from shields.io please.

@styfle
Copy link
Contributor Author

styfle commented Aug 9, 2018

How about https://badgen.net which is typically much faster?

@dougwilson
Copy link
Contributor

As long as all the badges are from the same place so they are consistent is the main goal. The secondary goal is to understanding the tracking policy of the site if you can link to that for badgen.net 👍

@dougwilson
Copy link
Contributor

I would suggest, though, before we change all existing badge (here and across every Express.js project so they are consistent), can you open a proposal for discussion at https://github.com/expressjs/discussions ?

@styfle
Copy link
Contributor Author

styfle commented Aug 9, 2018

Sure thing 👍
I created expressjs/discussions#66

@dougwilson
Copy link
Contributor

Thanks! So with the discussion for the different badge service over in it's own issue, the topic of the PR is back on hand:

(1) should this badge be added to the Express.js README?
(2) what is the specific value add?
(3) if yes, then should it be added to every single repo in the Express.js organizations?
(4) if no, should it be added to the badgeboard instead?

@styfle
Copy link
Contributor Author

styfle commented Aug 9, 2018

I have a feeling those questions are not meant for me to answer (correct me if I'm wrong) 😅

But you might find it interesting to see that Webpack has adopted this badge and it is visible for pretty much all webpack org packages as seen here: https://github.com/webpack/webpack#readme

@dougwilson
Copy link
Contributor

You may be able to help with (2), though you're welcome to answer any of them :) another doing something isn't reason enough by itself, of course, so what webpack is doing isn't exactly relevant 🤣

@wesleytodd
Copy link
Member

wesleytodd commented Aug 9, 2018

IMO:

(1) No
(2) I don't see a value in publicizing install size for this package. For something like webpack which is bloated beyond reason (10x the size of express according to the badges), sure, because it is a problem they have. But that is not a problem we have.
(4) Sure, I hardly ever visit the badge board so I don't have a reason not to lol.

@dougwilson
Copy link
Contributor

Webpack also looks very badge liberal while express is very badge conservative.

@LinusU
Copy link
Member

LinusU commented Aug 9, 2018

My personal preferences:

(1) No, I'm generally against any badges at all, unless they provide some very vital information
(2) I don't think it will add that much value since we aren't that heavy, and it's not something that I've ever seen someone complain about in regards to express
(4) Sounds like a good idea 👍

Sorry, not trying to be negative 🙈 I just personally don't see the value, doesn't mean that there is value for other though 🙌

@styfle
Copy link
Contributor Author

styfle commented Aug 10, 2018

I don't see a value in publicizing install size for this package. For something like webpack which is bloated beyond reason (10x the size of express according to the badges), sure, because it is a problem they have. But that is not a problem we have.

Do you have tests that check for size? Why do you think this won't be a problem in the future?

I don't think it will add that much value since we aren't that heavy, and it's not something that I've ever seen someone complain about in regards to express

People don't typically complain about something they are not measuring. No one use to complain about npm package versions being < v1.0.0 but then the badge started showing orange instead of blue and it helped make that a normal thing.


You're right, "install size" by itself is not enough info to decide to use a package or not but no single badge is enough.

What I am aiming to do is make this information visible that was once not visible.
It's a single data point in a matrix of many (see this table for an example).

Also, it creates a form of accountability.
Why do you display a badge for build status?
The core team already knows when a build fails so it's not for them.
It's for the users. Everyone who might use express gets to see this information front and center.

Lastly, you might be thinking...express will never get bloated and we never introduce bugs. Well take a look at this issue where TypeScript doubled in size (it happened more than once) microsoft/TypeScript#23339 (comment)

Edit: One more thing...did you know that express@4.16.0 is 50% larger than the previous version?

It's your README so you decide what goes in it but there's my answer to (2) above 😉

@amio
Copy link

amio commented Aug 10, 2018

In my personal actual experience, the install size badge might be more useful than build | passing, 🤪 let me explain:

I've seen a lot repo with build | failed but they are working fine, and a build | passing means not too much unless I checked the source to make sure it has a complete test suite, it's time consuming so I don't do this often.

On the other hand, install size are simpler and clearer. When I first saw a package I would check it's install size, and when I saw a new competitor I would compare it with the famous champions too, like micro vs koa vs express.

Overall, build status more like for developing, and install size more like user land concern, relatively speaking.

+1 for (1).

@LinusU
Copy link
Member

LinusU commented Aug 10, 2018

I'm very much +1 on removing the build status - passing. I mean, the checks have to pass in order to be able to merge to master, so it's never going to be anything but passing 😄

edit: wow, those TypeScript "bugs" are super interesting. Would it be easy to add installation size as a required check? That would be super cool, like if it increases more than a percent or two it has to be manually approved somehow 🤔 😎

@wesleytodd
Copy link
Member

I can get on board for a test which checks a PR install size against an install from the current major. The problem with a test like that is that it is fickle because some features might require that size change. Is anyone doing this kind of a test with success?

The problem I see with a badge is that it is more for show than function. That is why I could see a test being reasonable where a badge is not necessary.

@wesleytodd
Copy link
Member

Also, I did not know about the size increase, but if you look at the change log it makes sense:

https://github.com/expressjs/express/releases/tag/4.16.0

I imagine that was just the dependency updates and changes.

@amio
Copy link

amio commented Aug 10, 2018

BTW, I notice that TypeScript issue from microbundle, some day I suddenly found it's install size going up to 129MB, nearly 9x as webpack(14.8M) 🤯

That's not micro-bundle, that's huge-bundle. I checked it's releases, it support JSX since 0.3.0, and TypeScript in 0.4.0.

Now I have a new look at webpack. 😄

@styfle
Copy link
Contributor Author

styfle commented Aug 10, 2018

nearly 9x as webpack

Yeah webpack is actually much smaller than a lot of the competition after I started showing them Package Phobia but also because the project goals for parcel-bundler and microbundle to be the kitchen sink.

I did not know about the size increase, but if you look at the change log it makes sense

I am actually working on a PR to give better insight into which dependency is bloated. The only problem is I don't know Angular 🤷‍♂️

If you know someone who does, here's the PR: anvaka/npmgraph.an#27

I can get on board for a test which checks a PR install size against an install from the current major

I think this is possible with size-limit by setting options { webpack:false, gzip: false } but I haven't focused on this part (pre-publish) as much. Also see my research in this area by looking at the packagephobia#prior-art 👍

@dougwilson
Copy link
Contributor

The test proposals may want to be moved to another issue so they don't get lost in the badge discussion.

As for the badge points brought up, I can see what you're getting at -- build status, coverage is not like 100% relevant to the end users like an install size would be. Never really thought about it in that way before.

Along those lines, perhaps this is a useful badge, but with the caveat that we should divide up our badges into build data and end user, perhaps moving the build health badges down to the section about building and stuff and leave the top of the readme for these end user kind of badges?

If I were to think of the difference, i would think:

End user badges: version, downloads, engines, etc.
Build health badges: travis, appveyor, coverage, etc.

Thoughts fellows?

@amio
Copy link

amio commented Aug 12, 2018

I'm thinking same thing on badgen-service:

  • uptime, response time, license at top
  • dependencies, code style, code climate etc at "Developing" section.

The only concern is will it make the full page looks garish? 🤔

BTW, agree with @LinusU , if alls PR need to pass tests & building before merge into master, then the CI badge on homepage means very little.

@etroynov

This comment was marked as spam.

@dougwilson dougwilson added docs and removed discuss labels Mar 1, 2022
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

well, the readme has been rearranged, badges updated to badgen, and i think it's good to go

@styfle
Copy link
Contributor Author

styfle commented Mar 1, 2022

Thanks! The merge conflicts have been resolved 👍

@dougwilson dougwilson closed this in e8594c3 Mar 2, 2022
@dougwilson dougwilson merged commit e8594c3 into expressjs:master Mar 2, 2022
@styfle styfle deleted the patch-1 branch March 2, 2022 20:13
himanshiLt pushed a commit to himanshiLt/express that referenced this pull request Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants