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

Display Build Size Difference #340

Merged
merged 2 commits into from
Aug 4, 2016
Merged

Display Build Size Difference #340

merged 2 commits into from
Aug 4, 2016

Conversation

elijahmanor
Copy link
Contributor

This PR closes #326

Test Plan

  1. Run npm run build without a build folder
    • Should not show any difference
  2. Run npm run build with a build folder with no changes
    • Should show a green positive difference of zero
    • example: (+0 B)
  3. Run npm run build with a build folder after adding new code
    • Should show a red positive difference of some number
    • example: (+3 B)
  4. Run npm run build with a build folder after removing old code.
    • Should show green negative difference of some number
    • example: (-3 B)

build-difference-c

@ghost
Copy link

ghost commented Aug 3, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -77,7 +77,8 @@
"bundle-deps": "1.0.0",
"react": "^15.3.0",
"react-dom": "^15.3.0",
"react-test-renderer": "^15.3.0"
"react-test-renderer": "^15.3.0",
"recursive-readdir": "^2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in dependencies of this project.

@elijahmanor
Copy link
Contributor Author

Just signed the Contributor License Agreement, moved the devDependency to a dependency, squashed, and force pushed to this feature branch

@ghost ghost added the CLA Signed label Aug 3, 2016
@ghost
Copy link

ghost commented Aug 3, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

A few notes:

  • I don’t think yellow is the best choice for the build size itself. We use it for warnings, and it makes me feel uneasy about build now. 😄 Can we use cyan or some other color there? Maybe even white would work there now that we have diffs. We could tweak the color of filename if you don’t want too much cyan. Please try a few different stylings and see what works best given the meanings we assign to colors.
  • I’d like to display red for >= + 50 KB, yellow for + 0 ... 50 KB, and green for - ... KB.
  • I currently see (+ 0 KB) on rebuilds, this shouldn’t be happening. Diff should not be displayed on unchanged files.

console.error(err.message || err);
process.exit(1);
function sizeDifference(currentSize, previousSize) {
if (previousSize === undefined) { return ''; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Code style: please only use multiline blocks.

if (...) {
  ...
}

@elijahmanor
Copy link
Contributor Author

@gaearon Ahh, I didn't realize about the previous meaning of yellow. I changed it in hopes the size difference would stand out. I like your idea about color thresholds.

I wasn't sure what to do with a 2nd build when nothing changed, so I decided to show (+ 0 KB) to indicate that it was a good thing they didn't increase in size (Test Plan 2). However, I also understand not wanting to see anything at all, so I'll update that logic.

Sure, I'll update the multiline block. Thanks for the feedback. I'll work on those things tonight and update the PR. Do you want to me squash the commits or do you prefer keeping them separate so you can see the difference? Also, do you want me to rebase against master before I push my updates?

@alexzherdev
Copy link
Contributor

@gaearon Btw I think white should be avoided as some people might still have white as the terminal's background? 😃

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

Btw I think white should be avoided as some people might still have white as the terminal's background?

Sorry, I meant “default” color (no chalk). Great point 👍

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2016

Do you want to me squash the commits or do you prefer keeping them separate so you can see the difference? Also, do you want me to rebase against master before I push my updates?

Please rebase against master but don’t worry about squashing—GH will do that. I only ask to rebase in case I merge something conflicting in the meantime. I don’t expect to though.

@ghost ghost added the CLA Signed label Aug 3, 2016
@elijahmanor
Copy link
Contributor Author

Sorry, I meant “default” color (no chalk). Great point 👍

Cool, I'll keep that in mind. Thanks @gaearon @alexzherdev for the clarification

Please rebase against master but don’t worry about squashing

👍

@ghost ghost added the CLA Signed label Aug 3, 2016
@ghost ghost added the CLA Signed label Aug 4, 2016
@elijahmanor
Copy link
Contributor Author

Okay, @gaearon I made the adjustments you requested. Since I'm now only showing file-size difference for files that changed I needed to add strip-ansi to assist with the padding logic (to remove the unicode characters used by chalk).

I fetched/merged from upstream/master and rebased this feature branch based on that.

If you find anything else please let me know and I'll be happy to update this PR.

Thanks!

@ghost ghost added the CLA Signed label Aug 4, 2016
@gaearon gaearon merged commit 6b6d7ee into facebook:master Aug 4, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

Thank you!

@gaearon gaearon added this to the 0.3.0 milestone Aug 4, 2016
@gaearon gaearon modified the milestones: 0.2.2, 0.3.0 Aug 25, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 25, 2016

This was released in 0.2.2 but I forgot to add a changelog entry.
Doing it now.
Thanks again!

linhaobin pushed a commit to linhaobin/create-react-app-hb that referenced this pull request Jun 28, 2018
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display difference from the previous build in filesize
3 participants