Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PR diff comments for npm package changes #4511

Merged
merged 4 commits into from
Nov 30, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Nov 27, 2023

This PR adds "Diff changes to package" to the Tests GitHub Actions workflow to close #4258

This new workflow adds git diff comments for packages/govuk-frontend/dist in every PR

Diff comment examples

Have a look at 1x small and 1x big example:

PR diff comments follow the same approach as #4039

Example PR diff comment

@colinrotherham colinrotherham changed the base branch from main to build-dist November 27, 2023 14:50
Copy link

github-actions bot commented Nov 27, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 113.65 KiB
dist/govuk-frontend-development.min.js 38.28 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.78 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 73.09 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.64 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.27 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 69.41 KiB
components/accordion/accordion.mjs 21.67 KiB
components/button/button.mjs 4.7 KiB
components/character-count/character-count.mjs 21.24 KiB
components/checkboxes/checkboxes.mjs 5.83 KiB
components/error-summary/error-summary.mjs 6.09 KiB
components/exit-this-page/exit-this-page.mjs 16.08 KiB
components/header/header.mjs 3.9 KiB
components/notification-banner/notification-banner.mjs 4.54 KiB
components/radios/radios.mjs 4.83 KiB
components/skip-link/skip-link.mjs 3.81 KiB
components/tabs/tabs.mjs 9.66 KiB

View stats and visualisations on the review app


Action run for 8881365

Copy link

github-actions bot commented Nov 27, 2023

JavaScript changes to npm package

No changes found.

Action run for 8881365

Copy link

github-actions bot commented Nov 27, 2023

Stylesheets changes to npm package

No changes found.

Action run for 8881365

Copy link

github-actions bot commented Nov 27, 2023

Other changes to npm package

No changes found.

Action run for 8881365

@colinrotherham colinrotherham changed the title [SPIKE] Diff changes to dist for all PRs [SPIKE] Diff changes to package for all PRs Nov 27, 2023
@colinrotherham colinrotherham force-pushed the build-dist-diff branch 4 times, most recently from 0f64dbf to 1c37939 Compare November 27, 2023 21:16
@colinrotherham colinrotherham force-pushed the build-dist branch 3 times, most recently from ec8a02d to 122efff Compare November 28, 2023 10:18
@colinrotherham colinrotherham force-pushed the build-dist branch 3 times, most recently from fd8e14e to a6fc4c4 Compare November 28, 2023 13:30
@colinrotherham colinrotherham force-pushed the build-dist branch 3 times, most recently from f4af7a6 to 57453be Compare November 28, 2023 15:04
Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great - imho, more information is always good.

Coupla thoughts:

  • Are other devs OK with the increased noise (I say, let's merge and see...)
  • Are the comments always going to generate in order? For example, it's weird to see "Other changes" first on 🚀 Pre-release v5.0.0-internal.0 #4373
  • I wonder if it's worth rethinking the grouping down the line - feels slightly strange to see CSS and SCSS changes split up on Fix header product name wrapping onto new line #4498 , especially since they directly relate to each other. That could be a whole spiral of work though.
  • Another potential future idea: Can we merge in the size stats with these comments? Only highlight a size change if there's some diff in the relevant files? Should we have a comment for each of the files currently in the stats comment, if they have a difference?

# Using `origin/$GITHUB_BASE_REF` to avoid actually checking out the branch
# as all we need is to let Git diff the two references
bin/dist-diff.sh origin/$GITHUB_BASE_REF $GITHUB_WORKSPACE
git config user.name github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these needed because of the move to a workflow_call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is a long story

We know we want to use git diff for js-beautify formatting but the npm package isn't committed

Running git diff on uncommitted files

Since we need to build both the "before" (base) and "after" (head) npm packages:

Idea 1: Build into two directories and run git diff --no-index
Idea 2: Build + commit twice, then run git diff HEAD^ HEAD on the last 2 commits

Sadly we can't use ":(exclude)**/*.min.*" pathspec with git diff --no-index

But to commit each build we need user.name and user.email even though they never go anywhere

@colinrotherham
Copy link
Contributor Author

Thanks @domoscargin

Appreciate the code review

  • Are other devs OK with the increased noise (I say, let's merge and see...)

I'm happy with the noise to avoid accidental npm package changes creeping in such as:

  1. Browserslist caniuse-lite CSS changes like -webkit prefix removals
  2. Babel adding code transforms (or injecting polyfills)
  3. PostCSS or Sass changing the CSS bundle output
  4. Rollup changing the JS bundle output

We can remove the "No changes found" default in future perhaps?

☝️ Ah I hadn't noticed this but it'll be a side effect of racing them to the finish in:

// Run the comments in parallel to avoid that an early one failing prevents the others
// and use `allSettled` to avoid the promise rejecting on the first failure but wait

I won't merge this PR just yet until we get some more feedback

Like all the future ideas though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facilitate diffing of package when reviewing Dependabot PRs that update build
3 participants