Adds Danger and a rule showing build size differences #11865

orta · 2017-12-15T23:47:13Z

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.

Danger runs during your CI process, and gives teams the chance to automate common code review chores.

This provides another logical step in your build, through this Danger can help lint your rote tasks in daily code review.

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:

Adds Danger as one of the build steps during CI
Adds the same API token as React Native ( for @facebook-open-source-bot ) (example)

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.

gaearon · 2017-12-15T23:50:32Z

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 results.json file after yarn build which happens on the second Circle node.

orta · 2017-12-16T12:26:52Z

Yep, I can look at that

orta · 2017-12-16T17:20:54Z

@gaearon OK, so this now posts the table. Things worth considering:

Should this always show?
Should this be hidden behind markdown summary?
Can you think of ways to save horizontal space, so it could fit? ( maybe multiple tables?)
Are there things in particular that we could highlight? ( green / red / bold color )

gaearon · 2017-12-18T18:14:13Z

Should this always show?

It should show if there's non-zero difference in any of the files. It should filter out any bundles that haven't changed.

Should this be hidden behind markdown summary?

Maybe we can separate significant (>300 bytes post gzip to production bundles) changes from insignificant, and hide insignificant ones in a summary by default.

Can you think of ways to save horizontal space, so it could fit?

We can group them by bundle name. Something like

-----
react-reconciler
-----
umd_dev | ... 
umd_prod | ...
cjs_dev | ...
...
-----
react-dom
-----
umd_dev | ... 
umd_prod | ...
cjs_dev | ...
...
-----

Are there things in particular that we could highlight? ( green / red / bold color )

%?

gaearon · 2017-12-18T18:58:08Z

OK, I did a few updates on master that ensure the table printed at CI is accurate against master.

gaearon · 2018-01-02T18:32:52Z

dangerfile.js

+ *
+ * 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.


MIT license please :-)

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

gaearon · 2018-01-02T19:51:06Z

What’s missing here?

orta · 2018-01-02T19:56:44Z

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 💯

gaearon · 2018-01-02T20:04:40Z

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:

omit rows with 0 changes
hide table in a details tag
provide a one-line summary for React + ReactDOM UMD_PROD combined changes above the fold.

orta · 2018-01-02T20:09:40Z

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 👍

gaearon · 2018-01-02T20:13:48Z

If it reverts back to no difference in build size, then the message will be deleted.

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.

The rest sound good to me

Want to do it?

orta · 2018-01-02T20:19:31Z

Nah, I mean on every CI run, the message will be updated with the latest commit in the PR vs the diff against results.json from http://react.zpao.com/builds/master/latest/results.json.

I'll take a shot, I'm free for the next 45m

gaearon · 2018-01-10T15:27:18Z

Did you figure out why it uses wrong commit as a base?

orta · 2018-01-10T16:06:31Z

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 (danger.pr.head.sha) but now Danger uses the commits in the PR to find the parent for the earliest commit (danger.git.commits[0].parents[0]) which should mean it will only tell you the relative changes for your specific PR commit range.

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

orta · 2018-01-14T23:58:41Z

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 git status shows no changes, when I run node ./scripts/rollup/build.js --extract-errors however, I get results.json differences. Which is what I think we're seeing a difference in the results.

I've just shipped a commit that removes the --extract-errors - hoping that means this run means no difference.

gaearon · 2018-01-15T00:05:49Z

--extract-errors should run on the Circle build.

To clarify:

It is expected that --extract-errors size won't match the results.json stored in the repository for the given commit.
However, it should match the size of results.json from zpao's server for that commit since we upload results.json precisely after --extract-errors build.

If either of those is wrong then there's a bug in either uploading or downloading results.json to/from zpao's server. But --extract-errors is there intentionally.

orta · 2018-01-17T00:32:54Z

Found it, the lookup for bundle results weren't taking different bundleTypes (e.g NODE_DEV) into account so it would give differing results when the JSON was the same. Code: 1535536

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

gaearon · 2018-01-17T00:39:27Z

Seems like it's gone and this is good to go? 👍

orta · 2018-01-17T00:40:48Z

Yep, rebasing right now then that should be happy ~1 month anniversary PR.

gaearon · 2018-01-17T00:41:13Z

(Needs a rebase or merge)

orta · 2018-01-17T00:57:49Z

👍 it's ready

gaearon · 2018-01-17T01:49:35Z

Let's give this a try! Can always temporarily revert if something breaks.

esbena · 2018-01-18T13:13:59Z

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

gaearon · 2018-01-18T13:21:39Z

Mind fixing?

facebook-github-bot added the CLA Signed label Dec 15, 2017

orta force-pushed the add_danger branch 4 times, most recently from f346cfc to 4a08712 Compare December 16, 2017 17:05

orta changed the title ~~Adds Danger and an initial rule for large PRs~~ Adds Danger and an rule outputting build size differences Dec 16, 2017

orta changed the title ~~Adds Danger and an rule outputting build size differences~~ Adds Danger and a rule outputting build size differences Dec 16, 2017

orta changed the title ~~Adds Danger and a rule outputting build size differences~~ Adds Danger and a rule showing build size differences Dec 16, 2017

gaearon mentioned this pull request Dec 18, 2017

Upload build stats to the server #11880

Merged

gaearon mentioned this pull request Dec 18, 2017

Update results.json from master before the build #11882

Merged

orta force-pushed the add_danger branch from 4a08712 to 69fae23 Compare December 22, 2017 20:40

This was referenced Dec 22, 2017

Adds module and bundle type metadata to the rollup results json #11914

Merged

Fixes for danger pr vs danger WRT async code danger/danger-js#452

Merged

orta force-pushed the add_danger branch 3 times, most recently from d86b1c4 to 15b6864 Compare January 1, 2018 23:39

gaearon reviewed Jan 2, 2018

View reviewed changes

orta force-pushed the add_danger branch from 15b6864 to ea2df29 Compare January 2, 2018 19:23

orta force-pushed the add_danger branch from ea2df29 to 1949913 Compare January 2, 2018 20:50

orta force-pushed the add_danger branch 2 times, most recently from a5502d1 to 0001db0 Compare January 9, 2018 21:38

orta mentioned this pull request Jan 9, 2018

If a post only contains markdown, then Danger does not edit the comment danger/danger-js#475

Closed

orta mentioned this pull request Jan 14, 2018

Paginate all the issues danger/danger-js#479

Merged

orta force-pushed the add_danger branch 2 times, most recently from 1257d5a to 369c4a6 Compare January 14, 2018 23:21

Orta Therox and others added 6 commits January 16, 2018 19:34

Adds danger_js with an initial rule for warning about large PRs

abfb144

Signed-off-by: Anandaroop Roy <roop@artsymail.com>

[WIP] Get the before and after for the build results

2d84a5c

[Dev] More work on the Dangerfile

15ada00

[Danger] Split the reports into sections based on their package

a0d9b7a

Remove the --extract-errors on the circle build

56379cb

[Danger] Improve the lookup for previous -> current build to also inc…

8d9e9cb

…lude the environment

orta force-pushed the add_danger branch from 1535536 to 8d9e9cb Compare January 17, 2018 00:40

Fix rebase

284a53d

gaearon merged commit d8d7976 into facebook:master Jan 17, 2018

esbena mentioned this pull request Jan 18, 2018

Fix copy paste error for file size comparison #12040

Merged

gaearon mentioned this pull request Jan 18, 2018

Highlight production bundles in bold in the Danger integration comment #12043

Closed

eps1lon mentioned this pull request Feb 19, 2019

[ci] Use dangerJS to report bundle size changes mui/material-ui#14587

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Danger and a rule showing build size differences #11865

Adds Danger and a rule showing build size differences #11865

orta commented Dec 15, 2017

gaearon commented Dec 15, 2017

orta commented Dec 16, 2017

orta commented Dec 16, 2017 •

edited

gaearon commented Dec 18, 2017

gaearon commented Dec 18, 2017

gaearon Jan 2, 2018

orta Jan 2, 2018

orta Jan 2, 2018

gaearon commented Jan 2, 2018

orta commented Jan 2, 2018

gaearon commented Jan 2, 2018 •

edited

orta commented Jan 2, 2018 •

edited

gaearon commented Jan 2, 2018

orta commented Jan 2, 2018

gaearon commented Jan 10, 2018

orta commented Jan 10, 2018

orta commented Jan 14, 2018

gaearon commented Jan 15, 2018

orta commented Jan 17, 2018

gaearon commented Jan 17, 2018

orta commented Jan 17, 2018 •

edited

gaearon commented Jan 17, 2018

orta commented Jan 17, 2018

gaearon commented Jan 17, 2018

esbena commented Jan 18, 2018

gaearon commented Jan 18, 2018

Adds Danger and a rule showing build size differences #11865

Adds Danger and a rule showing build size differences #11865

Conversation

orta commented Dec 15, 2017

Adds Danger

gaearon commented Dec 15, 2017

orta commented Dec 16, 2017

orta commented Dec 16, 2017 • edited

gaearon commented Dec 18, 2017

gaearon commented Dec 18, 2017

gaearon Jan 2, 2018

Choose a reason for hiding this comment

orta Jan 2, 2018

Choose a reason for hiding this comment

orta Jan 2, 2018

Choose a reason for hiding this comment

gaearon commented Jan 2, 2018

orta commented Jan 2, 2018

gaearon commented Jan 2, 2018 • edited

orta commented Jan 2, 2018 • edited

gaearon commented Jan 2, 2018

orta commented Jan 2, 2018

gaearon commented Jan 10, 2018

orta commented Jan 10, 2018

orta commented Jan 14, 2018

gaearon commented Jan 15, 2018

orta commented Jan 17, 2018

gaearon commented Jan 17, 2018

orta commented Jan 17, 2018 • edited

gaearon commented Jan 17, 2018

orta commented Jan 17, 2018

gaearon commented Jan 17, 2018

esbena commented Jan 18, 2018

gaearon commented Jan 18, 2018

orta commented Dec 16, 2017 •

edited

gaearon commented Jan 2, 2018 •

edited

orta commented Jan 2, 2018 •

edited

orta commented Jan 17, 2018 •

edited