Add Dangerfile and provide bundle size info

[mAAdhaTTah]

Fixes #1787.

[github-actions]

No JS Changes

Generated by 🚫 dangerJS against 7b921a7

[mAAdhaTTah]

This is fairly rudimentary but basically does the thing. I can add additional comparisons to the table, if desired, adding comparisons to the source files and/or adding comparisons to non-gzipped versions.

The reason I didn't use a lib like bundlesize is they enforce a max per file or glob checked and I don't think the purposes here is to enforce a max so much as provide additional information for evaluating adding few features.

Open to comments/suggestions, but this is ready for review!

[RunDevelopment]

Looking good but there are still some things to improve.

Could you please add a summary and the absolute change? It's nice that know the relative change for each file, but the absolute changes (per file and in total) are also interesting.

[RunDevelopment]

Hmmm. Did the file size change? I'm pretty sure prism-core.min.js was at 2.93KB (both master and pull) when I first saw the message.

[mAAdhaTTah]

@RunDevelopment Yes, we went from 1024 -> 1000 for the calculation to make KB accurate.

[RunDevelopment]

Ahh, yeah. I'm stupid. Maybe I could also remember what I say and agree to. That'd be great.

[mAAdhaTTah]

lol don't be so hard yourself.

[mAAdhaTTah]

Ok, so need to do some testing w/ deleted files & remote forks, but before I dive into that, how does the output look? I narrowed it down to JS files in the plugins & components folders. Do we want CSS files too?

[RunDevelopment]

CSS files rarely change, so I don't think that's necessary. Unminified files shouldn't be included either (otherwise I will remove all comments to keep those file sizes down 😄).
Your initial approach (all .min.js files) seemed fine to me.

Regarding the output, I think it's fine.
A summary would be nice if more than 1 minified file changes. Though it might not be a good idea to just add up the gzipped differences. GZip will likely optimize between files in a real bundle (I'm thinking one file with all languages). I don't know what the best solution for this is.

[mAAdhaTTah]

A summary would be nice if more than 1 minified file changes.

Do you have something in mind for this? What sort of details would you want in a summary?

Though it might not be a good idea to just add up the gzipped differences. GZip will likely optimize between files in a real bundle (I'm thinking one file with all languages). I don't know what the best solution for this is.

Yeah, that makes sense. The goal isn't strictly to provide exact figure for all these use cases but to give us info on the tradeoffs we're making. Relatedly, when we get the benchmark tests in (I'm going to take a run through the open PRs over the coming week), we can add those here for some additional info.

[RunDevelopment]

A summary would be nice if more than 1 minified file changes.

Do you have something in mind for this? What sort of details would you want in a summary?

Nothing grand. Just the number of relevant files changed and the total number of bytes saved/gained.

The goal isn't strictly to provide exact figure for all these use cases but to give us info on the tradeoffs we're making.

In that case, we might as well just add up the gzipped differences for now and replace it with something better later.

Relatedly, when we get the benchmark tests in [...], we can add those here for some additional info.

Yeah no. The benchmark in its current form takes about 2 to 5 minutes (depends on the configuration) to run after all files have been downloaded. I don't want us to wait that long.

But we could make the benchmark start via a manual trigger. Idk, make a 🚀 reaction to the comment or something.

---

Something I hope to implement with this is what I said in #1758. If the PR changes the language definitions, the bot will post a link to the test page, so you can just try out the changes.

I also really like the example with test files here since we have the same situation. I'll probably make a few PRs after this one is merged.

[mAAdhaTTah]

Summary is added.

[mAAdhaTTah]

Ok, I think we're looking pretty good but let me know what you think! I'll fix up the failures once you sign off on the results.

[RunDevelopment]

I'm pretty sure the renaming-solution you implemented won't work.
Git's renaming detection is pretty good. It will even detect renames in files that marginally changed. This means that we can't assume that the content of the original and renaming file are the same. This is relevant because the diff in the summary will be wrong if we make this assumption.

In your test case, git wasn't able to detect the rename because you tested with minified files. Git's diff algorithm is line-based, so if you rename + change a one-line file, git won't be able to detect the rename. Please rename a file without changing it. Your current test case is just a deleted and an added file as far as git is concerned.

The problem with renames is that we don't know the name of the original file when using Danger. Danger only adds the renamed file in modified_files. You can't get the original file name. In #2616, I used git's API directly just so that I could disable git's renaming detection.

[mAAdhaTTah]

You're right in the abstract, but I was going for a best-guess solution that will produce a somewhat-valid output instead of erroring. I don't think we're going to end up renaming minified files that often, so I don't think we need to maintain a bunch of logic to make sure it's perfect. We could display a warning saying "hey whoops this was renamed, we don't know" but I think it'll be pretty clear that it'll look kinda like a new file but the diff is a rename.

Your current test case is just a deleted and an added file as far as git is concerned.

So basically, yes, and I think that's ok.

[RunDevelopment]

maintain a bunch of logic

So you agree to use the correct solution ;)

I mean, using git directly sounds scary but it's just 2 lines to get all changed files without renames.

const result = await git.diff(['--name-only', '--no-renames', 'master...']);
return result ? result.split(/\r?\n/g) : []; // return an array of changed files

These two lines mean that we don't have to worry about renames all. Renamed files are just one deleted file and one added file. Getting the file content is simple:

const [fileContents, fileMasterContents] = await Promise.all([
	fs.readFile(file, 'utf-8').catch(() => null),
	git.show([`master:${file}`]).catch(() => null),
]);

That's a lot simpler than the current error-handling solution, isn't it?

[mAAdhaTTah]

Fair enough. Implemented that. Kept it catch(() => '') and added silent to simplegit cuz the git show was erroring to the console.

[mAAdhaTTah]

Alright, I think that gets us there.

[RunDevelopment]

Just some minor nits and then I think we're done.

Also, please add dangerfile.js to .npmignore. (Or maybe we could add a files section to package.json and ditch .npmignore instead? Might be easier looking forward.)

[mAAdhaTTah]

Ok, I've added your last suggestions, removed the failures, and squashed everything down. Are we good to go?

[RunDevelopment]

@mAAdhaTTah This is something I wanted to ask: Why do you always squash the commit? I always "squash and merge" on GitHub anyway.

[mAAdhaTTah]

@RunDevelopment By default, when GitHub squash & merges, the commit message is going to be a list of all the commits squashed together, which I don't really like as a commit message. I could rewrite the message as I merge, but in the past, I've forgotten to do that, and since I'm thinking about the commit message when I'm making the commit, I prefer to combine them into the one commit, with the message that I want, while I'm committing.

In this case in particular, because there was stuff in the commits I wanted to remove, I actually rolled back all of the commits locally and readded the commit with just the real changes.