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 badge to display install size #24282

Closed
wants to merge 1 commit into from
Closed

Conversation

styfle
Copy link
Contributor

@styfle styfle commented May 20, 2018

This adds a badge to the README.md file to display the install size of the npm package.

This will catch bugs such as the one described in #23339

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2018

ppl on the team rarely look at the badges.. so i do not think this is an effective way of measuring output size. I would say a better solution is to do this in jake LKG/gulp LKG which we run manually, we can list the size of the files with a warning message if it is above 10% of the size before the change.

@styfle
Copy link
Contributor Author

styfle commented May 22, 2018

The badge is also useful for users, not just developers of TypeScript

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2018

I am not sure i agree about the usefulness of the size, specially that it is something we are not optimizing for. for instance we could minify the output, but we are not.. and developers can easily tell how big the package with a quick test.

@styfle
Copy link
Contributor Author

styfle commented May 22, 2018

developers can easily tell how big the package with a quick test

The reason why I wrote Package Phobia is because it's not that quick to test for this, especially for users who are using slow internet connections.

  • 3G internet speed is roughly 50 kB per second
  • 35935744 bytes / 50000 bytes per second = 718 seconds

That's about 12 minutes to download TypeScript over 3G!

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2018

Developers can always use hit https://packagephobia.now.sh/result?p=typescript to know how big is the package.. Seems like easy test for me. No need for a badge.

@styfle
Copy link
Contributor Author

styfle commented May 22, 2018

The badge creates some accountability because users will see it...even if they have never heard of Package Phobia.

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2018

As I noted earlier, this is not something we are willing to commit to, we are not optimizing for package size at the moment.

If you would like to help us keep the package size in check, we would appreciate a change to the jake LKG/gulp LKG commands to check the total size of the lib folder and flag any unexpected increase there.

@styfle
Copy link
Contributor Author

styfle commented May 24, 2018

What is the max size that should cause the build to fail?

@mhegazy
Copy link
Contributor

mhegazy commented May 24, 2018

I would say 10% than the current size.

@styfle
Copy link
Contributor Author

styfle commented May 24, 2018

Ok so I can put a hard limit at 39529266 bytes.

Should I modify run-sequence or add a new check in "LKG"?
https:/Microsoft/TypeScript/blob/master/Gulpfile.js#L585

@mhegazy
Copy link
Contributor

mhegazy commented May 24, 2018

I would say compute the size of the lib folder before the command runs, compare it to that after the command runs, and if the diffrence is more than 10% raise an error.

@styfle
Copy link
Contributor Author

styfle commented May 24, 2018

I saw that the React team uses a dangerfile to add a comment on each PR with the change in size.

It looks like this: facebook/react#12564

This might better than a gulp task because it lets the reviewers decide if it should be merged or not.

Thoughts?

@mhegazy
Copy link
Contributor

mhegazy commented May 24, 2018

Do not think we want to pay the setup cost for a new tool. a check in LKG would be sufficient.

@styfle
Copy link
Contributor Author

styfle commented Jun 6, 2018

@mhegazy I created PR #24712

Let me know if I did it right or if it needs to be revised, thanks 😅

@mhegazy
Copy link
Contributor

mhegazy commented Jun 8, 2018

Closing in favor of #24712. thanks @styfle!

@mhegazy mhegazy closed this Jun 8, 2018
@styfle styfle deleted the patch-1 branch June 8, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants