[Danger] Minor fixes #12606
Merged
[Danger] Minor fixes #12606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
bvaughn
approved these changes
Apr 11, 2018
|
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. |
This was referenced Apr 17, 2018
This was referenced Apr 19, 2018
rhagigi
pushed a commit
to rhagigi/react
that referenced
this pull request
Apr 19, 2018
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.