-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Adds Danger and a rule showing build size differences #11865
Conversation
Wow, thanks for sending a PR! I'm not sure sure we have a use for that particular rule though. What would've been much more valuable is to output the build size difference. Is that something we could do? The data should be available in |
Yep, I can look at that |
f346cfc
to
4a08712
Compare
@gaearon OK, so this now posts the table. Things worth considering:
|
It should show if there's non-zero difference in any of the files. It should filter out any bundles that haven't changed.
Maybe we can separate significant (>300 bytes post gzip to production bundles) changes from insignificant, and hide insignificant ones in a summary by default.
We can group them by bundle name. Something like
%? |
OK, I did a few updates on master that ensure the table printed at CI is accurate against master. |
d86b1c4
to
15b6864
Compare
dangerfile.js
Outdated
* | ||
* This source code is licensed under the BSD-style license found in the | ||
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIT license please :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah!
Sure, (I've been hacking on this slowly over the last two days) I am a bit surprised by the build failures but I think this is pretty much good to go once those are figured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
What’s missing here? |
Ah, nothing now it's green. 👍 The markdown will now only show the tables for modules if any of the values are not nil. It should be easy to shuffle that data around (in terms of what the table row order etc) but this is basically 💯 |
Do I understand correctly that updating this PR will update the table in the comment above? I think there are three changes I’d like to see before merging:
|
Yep, as new commits happen the PR will be updated with the new table. If it reverts back to no difference in build size, then the message will be deleted. The rest sound good to me 👍 |
Why does it have the difference though? Is it because the PR runs against master rather than merge base? I guess that’s a bigger issue than I initially though then.
Want to do it? |
Nah, I mean on every CI run, the message will be updated with the latest commit in the PR vs the diff against I'll take a shot, I'm free for the next 45m |
a5502d1
to
0001db0
Compare
Did you figure out why it uses wrong commit as a base? |
I did, but I'm still not sure why on the results aren't "no changes" In the previous examples danger was using the head of master on facebook/react ( Sorry it's dragging, I'm on the (hopefully) last day of shipping an app we've been working on for 6 months so my time management has been pretty strained |
1257d5a
to
369c4a6
Compare
OK, so I did some debugging, I did git checkout a442d9bc082878ccf66872b1eeed3465927801b6
node ./scripts/rollup/build.js And there were no differences on the results, I went back to this PR: git checkout add_danger
node ./scripts/rollup/build.js and I've just shipped a commit that removes the |
To clarify:
If either of those is wrong then there's a bug in either uploading or downloading |
Found it, the lookup for bundle results weren't taking different bundleTypes (e.g We should see the Danger message disappear, because there is nothing to say as the build sizes are unchanged in this PR. |
Signed-off-by: Anandaroop Roy <roop@artsymail.com>
…lude the environment
Seems like it's gone and this is good to go? 👍 |
Yep, rebasing right now then that should be happy ~1 month anniversary PR. |
(Needs a rebase or merge) |
👍 it's ready |
Let's give this a try! Can always temporarily revert if something breaks. |
I believe this PR contains a copy/paste error: https://github.com/facebook/react/blob/master/dangerfile.js#L109 r => r.prevGzipSizeChange !== 0 || r.prevGzipSizeChange !== 0 should be r => r.prevFileSizeChange !== 0 || r.prevGzipSizeChange !== 0 |
Mind fixing? |
Adds Danger
Adds danger-js with a single rule taken from the React Native Dangerfile. I am less familiar with React, the repo, than I am with the process around React Native. So I didn't want to intrude.
Which means that you can start writing simple scripts to handle "Add a CHANGELOG" or "No merger commits please" basically. @hramos wrote a bunch in RN, and there is an API reference here. You can test changes locally by running
node node_modules/.bin/danger pr https://github.com/facebook/react/pull/[ID]
.This PR:
I'm not 100% certain that I added it into the right section in test_entry_point.sh - happy to change that if you have better ideas.