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

[Danger] Minor fixes #12606

Merged
merged 4 commits into from
Apr 11, 2018
Merged

[Danger] Minor fixes #12606

merged 4 commits into from
Apr 11, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 11, 2018

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.

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.
Copy link
Contributor

@bvaughn bvaughn left a 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. 👍

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 11, 2018

Eh. So this one is because we never clean up deleted bundles from the list. 😄
I'll clean up the list now but we need to figure out some way to do this.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 11, 2018

What is "create-component-with-subscriptions"? 🤔

@bvaughn
Copy link
Contributor

bvaughn commented Apr 11, 2018

Ok, that makes sense. Funny!

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 11, 2018

@bvaughn That's your first naming attempt. :-)

@bvaughn
Copy link
Contributor

bvaughn commented Apr 11, 2018

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.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 11, 2018

I think this should work, let's give it a try.

@gaearon gaearon merged commit c27a998 into facebook:master Apr 11, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants