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

Remove built govuk-frontend from committed files #3498

Merged
merged 13 commits into from
May 18, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Apr 12, 2023

This PR removes the govuk-frontend build output we use for npm publish

This was known as the ./package directory until it moved to ./packages/govuk-frontend/dist in:

We don't need to commit it as we rebuild it during GitHub Actions tests

But it also addresses other concerns found during the performance work:

  1. Our build outputs ESM files but we only test the UMD bundles
  2. Our build outputs Sass with autoprefixer, but we use ./src instead
  3. Our build outputs Nunjucks templates, but we use ./src instead
  4. Our build outputs JSON component data, but we use ./src instead
  5. Our build outputs font and image assets, but we use ./src instead
  6. Review app ignores ./package and builds its own files from ./src

Point 1) has been resolved in #3491
Point 4) has been deemed acceptable for now

Once deleted, we can import from the built govuk-frontend instead

@colinrotherham colinrotherham requested a review from a team as a code owner April 12, 2023 10:17
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 12, 2023 10:17 Inactive
@colinrotherham colinrotherham self-assigned this Apr 12, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 12, 2023 15:11 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 13, 2023 17:24 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 13, 2023 17:48 Inactive
@colinrotherham colinrotherham force-pushed the review-app-user branch 2 times, most recently from 08ec480 to bfb6bab Compare April 18, 2023 15:14
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 20, 2023 08:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 April 20, 2023 13:20 Inactive
@colinrotherham colinrotherham force-pushed the review-app-user branch 2 times, most recently from 08f1d4a to dd706d3 Compare April 21, 2023 16:00
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 May 18, 2023 11:55 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 May 18, 2023 13:00 Inactive
But rebuild them during `predev`, `pretest` and `heroku-postbuild`
This breaking change moves the npm published `package/dist/package.json` to `package/package.json`

Package exports will be updated in another commit
We currently ship with Node.js v4.2.0 support in `package.json`

But ESLint is reporting that we need:

* Node.js v14.0.0 for 'fs/promises'
* Node.js v7.6.0 for Async functions

Plus we’d need Node.js v12.19.0+ for wildcard package exports
Avoids breaking changes for Node.js (and bundlers that support package exports)

Note: Sass load paths will still need the new `dist/` prefix
But excludes component data `*.yaml` which only exists in source
We can look at Node.js warnings on `npm install` in another PR but maintain ESLint Node.js compatibility checks using `settings.node.version`
We’re happy to make the `/dist` suffix a breaking change for v5
Our local packages don’t exist on https://registry.npmjs.org and can be skipped when outside npm workspaces
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3498 May 18, 2023 13:12 Inactive
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Worked OK for dev (including watch), build:package as well as using npm link or a pre-release branch to use it in the Prototype Kit 🎉

@colinrotherham colinrotherham changed the base branch from main to move-config-prototype-kit May 18, 2023 13:42
Base automatically changed from move-config-prototype-kit to main May 18, 2023 13:43
@colinrotherham colinrotherham merged commit 0f42391 into main May 18, 2023
20 checks passed
@colinrotherham colinrotherham deleted the delete-package-dist branch May 18, 2023 13:49
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency dependencies Pull requests that update a dependency file interoperability
Projects
Development

Successfully merging this pull request may close these issues.

Make the review app consume govuk-frontend Move src and built package files under the same package
4 participants