Add describe-assets option

[mariaines]

My repo has thousands of assets that makes the 'Unchanged' section really unwieldy (especially in emails where they are not collapsed). This PR adds a describe-assets option to opt-out of showing unchanged assets (changed-only) or all asset info (none) (useful if you have hashed chunks)

[camchenry]

@mariaines Thank you so much for contributing! We really appreciate your contribution to this project.

I have reviewed the code and and I think that this looks like a good addition to have. I posted two small-ish suggestions: one for adding a test for the all option and a small type update. I also posted a comment discussing what configuration here might look like in the future if you have any thoughts, but I don't expect anything actionable from that for now.

[camchenry]

(not blocking) Perhaps a follow-up change we could make in a follow-up PR is allowing the statsFields array to be more customizable, so we don't need to pass an option specifically for just the unchanged part. For example, allow customizing each element of the array:

export function printAssetTablesByGroup(
  statsDiff: Omit<WebpackStatsDiff, 'total'>,
  enabledStatsFields: {
    added: boolean
    removed: boolean
    bigger: boolean
    smaller: boolean
    unchanged: boolean
  }
): string {
  const statsFields = Object.keys(enabledStatsFields).filter(
    (field): field is keyof typeof enabledStatsFields =>
      enabledStatsFields[field as keyof typeof enabledStatsFields]
  )

Then using it something like:

printAssetTablesByGroup(statsDiff, {
  added: true,
  removed: true,
  bigger: true,
  smaller: true
  unchanged: describeAssetsOption === 'all'
})

Not a change we need to make here, but just typing it out as something for us to consider 🤔

[mariaines]

Hi @camchenry thanks for the quick review!

I like this idea but was struggling to figure out how best to present these as options at the action level. Some ideas:

1. individual boolean options like describe-added-assets, describe-removed-assets, describe-bigger-assets, etc
2. a string option with space-separated fields, e.g describe-assets: "added removed unchanged" (and also support "all", "none", "changed-only" keywords?)
3. a keyword option (like I have now) with options all, none, added-and-removed-only, bigger-and-smaller-only, changed-only, unchanged-only (with the idea that you'd rarely want e.g. added but not removed)

Thoughts? I think I'm leaning towards 2, but lmk if you have an opinion!

[mariaines]

@camchenry I went with 2 :) LMK what you think!

[camchenry]

@mariaines Seems fine to me, we can always update it later if we need to. Of those 3 options, I agree that option 2 seems the most straightforward to configure.

[camchenry]

Just had some small suggestions, but otherwise I think this is a great addition 👍

[camchenry]

🎨 would it make sense to use expect(...).toThrow() here?

Suggested change

	try {
	getDescribeAssetsOptions('unsupported options')
	fail('should fail with unsupported options')
	} catch (e) {
	// pass
	}
	expect(() => getDescribeAssetsOptions('unsupported options')).toThrow()

[camchenry]

🎨 Could we get away with just initializing this once? Can't tell right away if there's any mutation going on here or something

Suggested change

	let statsDiff: WebpackStatsDiff
	beforeAll(async () => {
	statsDiff = getStatsDiff(
	await readJsonFile('./__mocks__/old-stats-assets.json'),
	await readJsonFile('./__mocks__/new-stats-assets.json')
	)
	})
	const statsDiff = getStatsDiff(
	await readJsonFile('./__mocks__/old-stats-assets.json'),
	await readJsonFile('./__mocks__/new-stats-assets.json')
	)

[mariaines]

Unfortunately we can't do that since describe() can't be async: jestjs/jest#2235

beforeAll does indeed only run once before all the test cases within the describe though, unlike beforeEach, so it's ugly but should be correct 👍🏽

[mariaines]

Oh, I see now you are probably wondering if we can change the let to a const. Not sure of a better way around this but lmk if you think of one!

[mariaines]

@camchenry sorry for the long delay, I went out of the country :) thank you for the review!

[mariaines]

Thanks for reminding me of this @mattcosta7 ! Merged from main and ran npm run all, would love to get this in

[mattcosta7]

Thanks for reminding me of this @mattcosta7 ! Merged from main and ran npm run all, would love to get this in

thank you for this - I pulled these changes into #563 and updated the workflows there. Will get 👍 on this and get it landed soon!

[mattcosta7]

this is shipped in https://github.com/github/webpack-bundlesize-compare-action/releases/tag/v2.1.0