search

mainmatter / ember-asset-size-action

Comment with the diff for the asset sizes on Pull Request
MIT License
22 stars 15 forks source link

Account for deleted files when diffing assets #41

Closed yanglinz closed 1 year ago

yanglinz commented 1 year ago

Currently, deleted files (files that exist on the trunk branch but no longer exist in the PR branch) are ignored in the final asset size diff. This PR changes the asset diffing helper so that deleted files are included.

mansona commented 1 year ago

Hey thanks for the contribution 🎉

Am I right in saying that this change would consider a deleted file as one that has gotten smaller? It would be good to see what the buildOutputTest.mjs would look like for the change that you want to make here.

also I created an issue that might make the requirement a bit clearer: https://github.com/mainmatter/ember-asset-size-action/issues/65

yanglinz commented 1 year ago

Yep you're right, as implemented it would not distinguish files that got smaller vs deleted. I agree that having the distinction would be ideal 👍

Thanks for writing up the issue and clarifying the requirements - I'll tweak the PR to address it!

yanglinz commented 1 year ago

@mansona I've tweaked my PR to distinguish between files that got smaller vs files that got deleted in a way that should satisfy https://github.com/mainmatter/ember-asset-size-action/issues/65. I also updated the tests in buildOutputTest.mjs as suggested. Let me know if my approach makes sense to you or if you had something else in mind!

mansona commented 1 year ago

oh I just pushed the updated dist to your branch there too, essentially you have to run npm run build and commit the dist folder since https://github.com/mainmatter/ember-asset-size-action/pull/43

This is a bit of a pain, but it's a lot better than having to commit all of node_modules 🙈