symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
858 stars 315 forks source link

Add CI workflow to compute diff between files dist files #2269

Closed Kocal closed 3 weeks ago

Kocal commented 1 month ago
Q A
Bug fix? no
New feature? no
Issues Fix #...
License MIT

🚨 Before merging, we should drop the commit that modify files (in order to generate the diff table)

This PR is purely internal, and aims to display the assets/dist/*.{js,css} files diff between 2.x and a pull-request. Similar to https://github.com/marketplace/actions/pkg-size-action, but fully internal.

I wanted a tool that display dist files size diff for each pull-request, because I was a bit afraid of changes done in https://github.com/symfony/ux/pull/2160.

When a PR is opened, it check dist files between the base branch (2.x) and the pull request, and it create a GitHub comment. The comment is created by https://github.com/marocchino/sticky-pull-request-comment, and is automatically updated depending of the check state. If any diff between dist files, then a table is displayed, with a line per file. It shows the original size and compressed (gzip and brotli) sizes, and also a difference in %.

States

Currently not working on this repository, but you can see them on https://github.com/Kocal/symfony-ux/pull/1

Capture d’écran 2024-10-13 à 11 45 10

When opening a PR

When an issue happened

Capture d’écran 2024-10-13 à 11 45 19

When there is no difference between base and PR

Capture d’écran 2024-10-13 à 11 25 31

When there is difference between base and PR

image
Kocal commented 1 month ago

It looks like there are permissions issues, even after added permissions.pull-requests: write like mentioned in the documentation:

image

Even if I use permissions: 'write-all' it does not work either:

image

I suspect a configuration at organization-level...?

@kbond do you think you can do something about that please? 🙏🏻

/cc @nicolas-grekas or maybe you Nicolas since you setup the same action on Symfony recipes repositories :)

Thanks!

kbond commented 1 month ago

Shouldn't this job only run if a dist file has been changed?

Kocal commented 1 month ago

Shouldn't this job only run if a dist file has been changed?

Fixed by using the following config:

on:
  pull_request:
    paths:
      - 'src/*/assets/dist/**'
      - 'src/*/src/Bridge/*/assets/dist/**'

I will push soon, thanks

Kocal commented 1 month ago

I've changed the table rendering after @javiereguiluz's comment, it now looks like this:

image

We still need to find out why there is this Resource not accessible by integration error, and we are good IMO

javiereguiluz commented 1 month ago

@Kocal I like a lot what you are doing here. Thanks.

Some additional comments ... but I could be wrong, so feel free to ignore them:

Thanks!

Kocal commented 1 month ago

Thanks Javier, the more we speak about Brotli and the more I think we can remove it (btw pkg-size-action can display Brotli sizes). Gzip is the most common compression standard for the moment, while Brotli is — I believe — still a "niche" thing. Brotli users know that their files will be smaller than gzipped files.

I agree for percentage rounding.

When it comes to putting percentages first, I don't really agree with you. Yes, percentage differences are important, but when the browser goes to download a resource, it's the size of the resource that matters, not the percentage difference. If a file is reduced from 1 MB to 500 KB, we gain 50%, but more importantly we save 500 KB (and that's a really huge gain). If a file is reduced from 10 Kb to 5 Kb, you also gain 50%, but you only save 5KB (which still has an impact, but a much smaller one). But then, your point of view is valid too and I can understand it, I think here it's two different ways of thinking :)

Kocal commented 1 month ago

Thanks for your reviews, the diff table now looks like this:

image
javiereguiluz commented 1 month ago

Looks very nice!! Thanks a lot Hugo.

kbond commented 1 month ago

This looks great!

@Kocal:

  1. Can you revert the temporary commit?
  2. The proper permissions are still todo?
Kocal commented 1 month ago

@kbond yes there are still permission issues, and I can revert the temporary commit (when permissions will be fixed, otherwise we won't see the comment)

Kocal commented 3 weeks ago

I've reworked the workflow in two dedicated workflows, hoping for permissions issues to be gone. We won't have comment before the diff is generated, or if a failure happened.

I reverted the commit that modify dist files for testing purposes. I'm merging ASAP