Add threshold option

[aduth]

Having used compressed-size-action for some time in the WordPress/gutenberg project, I've come to the realization that more-often-than-not, I subconsciously ignore the bot's comments. This got me to thinking: What do I want from this bot? To which I consider the following use-cases:

• To flag abnormally large changes in size, particularly when someone may be introducing a new dependency that inflates the built project size (example).
• To validate my own efforts targeted specifically at reducing bundle size (example).

Both of these use-cases could be satisfied by adding an option to specify a threshold at which a comment should be created. This is what is proposed with these changes.

A couple extra notes:

• Given that the action is responsible not only for creating comments, but also updating them, the condition was limited to occur only when a comment would otherwise be created. This ensures that if there was a previous comment reporting a large size change, subsequent commits to the pull request to bring that size difference back down would be reflected in the comment.
• It occurs to me now that a possible concern with this approach could arise in that: Should a developer be confident in lack of comment signaling "Your changes do not have a substantial impact!" vs. "The comment was unable to be created" (permissions, etc). I suppose this could depend if you view the action as optimized toward creating (default) vs. only reporting abnormalities (edge cases).

[dstaley]

Should a developer be confident in lack of comment signaling "Your changes do not have a substantial impact!"

One possible tweak would be to use the threshold to calculate whether or not a file is "changed" rather than whether or not to post the comment. For example, if a file's delta was less than the threshold, the action would report that the file was unchanged, but still display the actual change delta. I think this would be a good compromise rather than simply not reporting, which has the issues you mentioned.

[developit]

All very good points, thanks for such a well-articulated explanation.

At minimum we could definitely ship @dstaley's option - right now there is already a per-file threshold, it's just hard-coded as 0.

The comment creation paradox is a tough one for sure. Perhaps as a workaround in the time being, maybe updates below the threshold could fully collapse the comment? I believe that's doable via the API. If not, the whole comment could be updated to be wrapped in <details>.

[aduth]

Thanks for the feedback, @dstaley and @developit !

To clarify, would the suggestion be to treat this similarly to the current "View Unchanged" collapsing (example)? Optionally as a separate (or renamed) grouping "View Below Threshold (##kB)" ?

Incidentally there's been a bit of ongoing feedback about this action from contributors to our project, and various thoughts about problems (like <details> not collapsing in email notifications) and possible solutions.

[dstaley]

Personally I'd just consider anything below the threshold as unchanged, and thus put it in the unchanged table.

[developit]

@dstaley same here.

[aduth]

Sorry y'all for the delay. I've been meaning to revisit this one. I like the idea to consider threshold as treated similar to what exists already with the behavior of a change that amounts to zero difference.

I see the approval. I'm not attached to getting this in in the current form. It might be an improvement, so if it's something you'd like to ship as-is, that's great. I'd also be interested to making the suggested changes. I've just yet failed to devote the time to seeing it through 😬

[developit]

@aduth hey, no worries. I approved it but then re-read the discussion and decided to release v2 without this for now since I couldn't decide.

[dstaley]

@developit I'm willing to jump in and take a stab at implementing this! Before I do though, I'd love to add tests for the diffTable function at a minimum to ensure things work as expected. I have some free time now to tackle that. Do you have a preferred testing library?

[developit]

@dstaley that would be really great. I had originally intended to move diffTable and the other independent helper functions out into their own module so that they could be unit tested.

I'm fine with any test runner really. I mainly use Jest or Mocha.

[developit]

My hope for what we could do here would be basically this:

for each run:
    if `threshold` option is null or an empty string:
        no change from today
    else:
        compute sizes
        if size delta is greater than threshold:
            same as today
        else:
            if there is already a comment:
                update the comment to collapse all rows
            else:
                do not create the comment

[DerekTBrown]

@aduth do you have plans to work on this? If not, I would be willing to jump in.

[aduth]

Hey @DerekTBrown , I admit I totally forgot about this one. If you're keen to see it over the finish line, I'm more than happy to let you take the charge. Dunno how best to manage that logistically, but if it's easiest for me to close this out and you can open a new pull request with anything useful from the changes here, happy to do that.