-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
[Danger] Minor fixes #12606
[Danger] Minor fixes #12606
Conversation
This was temporarily necessary in the past because we didn't have the logic that downloads actual *merge base* stats. We do have that now as part of the Danger script. So we can remove this.
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.
Both of these seem like good changes. 👍
Eh. So this one is because we never clean up deleted bundles from the list. 😄 |
What is "create-component-with-subscriptions"? 🤔 |
Ok, that makes sense. Funny! |
@bvaughn That's your first naming attempt. :-) |
Yah, I didn't realize it was a left over. I thought something was still publishing it and I was confused about what or how. |
I think this should work, let's give it a try. |
* Don't download bundle stats from master on CI This was temporarily necessary in the past because we didn't have the logic that downloads actual *merge base* stats. We do have that now as part of the Danger script. So we can remove this. * Use absolute threshold for whether to show a change * Download master stats, but only for other master builds * Rewrite sizes
The first commit removes the step that downloads fresh stats on CI. It should be unnecessary now because Dangerfile already does it more accurately—by downloading the stats for merge base.
This should fix the issue that caused newly added bundles to show up in recent unrelated PRs like here. It was happening because stats from master already included this bundle, but stats from the mergebase didn't. During the first build, the stats from master were "amended" with new data, but Danger's comparison thought those new rows were added by the build (whereas they were added by the first step that downloads stats from master). By removing the first step we shouldn't get false positives for recently added bundles (except the rarer case where a PR's merge base is older than the most recent release that updates the sizes on master). TLDR: there should be less noise in Danger's comments.
In the second commit, I changed the threshold to be absolute (100 bytes after gzip, or 300 bytes before gzip). This is because "1%" means wildly different things for, for example, ReactDOM and React. So it felt a bit unpredictable when the package would show up. Should be more consistent now.