andresz1 / size-limit-action

Compare the real cost to run your JS app or lib to keep good performance in every pull request
ISC License
451 stars 84 forks source link

added ignore_identical input for cleaner PRs #18

Closed dcwither closed 4 years ago

dcwither commented 4 years ago

Proposal: Ignore Identical Changes

Hi, thanks for building such a useful tool to measure bundle size differences! I've been using this for a little while so far, and found that the only issue I have is with duplicate reports when I push to the PR multiple times.

I've forked your repo and created a suggestion for how this might be mitigated. My changes aren't exactly as clean as I'd I'd like for a final version, but hopefully it's enough to get the idea across.

I think ideally this would be a GitHub app which can then run a check instead of a Review, which simply gets overwritten, but that's a slightly heavier handed approach.

andresz1 commented 4 years ago

Hi @dcwither I'm glad it has been useful 🙌 Thank you so much for opening this PR 🎉, I love the idea. I'd prefer to go for the second approach that you described. The action checks if a review has already been posted and if so it updates it. I think that this could be achieved mixing what you did and octokit.pulls.updateReview. What do you think ?

If you have time, feel free to update your PR! that would be awesome. If not, I'll try to merge your PR and create a new one with that during the weekend!

dcwither commented 4 years ago

I'd forgotten about update I think update can be a good way to go, it's mostly a matter of preference due to some tradeoffs

Update Review

Pros

Cons

Ignore Identical

Pros

Cons

I'm happy to work more on this, though I'll probably continue on Tuesday (holiday weekend).

andresz1 commented 4 years ago

Good comparison between the two options! thank you for that @dcwither. I think that both could be useful. What if we provide both options with one of them by default ? For example:

update_review:
    required: false
    description: 'whether or not identical reviews should be reposted on rebuild'
    default: false

If it's enabled we update the review and if not we ignore identical by default ?

dcwither commented 4 years ago

Digging into it more now.

andresz1 commented 4 years ago

Great @dcwither! let me know if you need help!

dcwither commented 4 years ago

I've put an updated version that provides the options to update/create new in #20. It's largely untested, but I'm not certain when I'll have more time to test it more robustly. If you'd like to take a look at it and provide feedback I can try to revisit a little down the line.

If you'd really like to see this go in sooner rather than later, you're more than welcome to pick up the work from here. Thanks for the feedback so far.

andresz1 commented 4 years ago

Thank you so much @dcwither! I'll try to continue your work during this week!

andresz1 commented 4 years ago

@dcwither Closing this one in favor of #25. Please let me know if you have any other idea or issue. Thank so much for the help!