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

Update Review app to import govuk-frontend via local package #3491

Merged
merged 9 commits into from May 4, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Apr 4, 2023

This PR makes the Review app a user of govuk-frontend as part of:

For the performance work all source code and build artifacts can now be resolved from a single package

Additionally, the Review app now follows our own documentation:
https://frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/

These areas in particular:

Import CSS, assets and JavaScript

  1. Simplify Sass import paths
  2. Import JavaScript using a bundler

Which means the Review app now imports govuk-frontend ESM JavaScript for the first time, without having to navigate to ../../../src sources or rely on a separate review-specific UMD bundle

Package directory changes

Previously npm link only resolved the pre-built packages files but they're now consolidated:

Before

├── app
│   ├── src
│   └── dist
└── package
    ├── govuk
    ├── govuk-esm
    ├── govuk-prototype-kit
    ├── package.json
    └── README.md

After

└── packages
    ├── govuk-frontend
    │   ├── src
    │   │   ├── govuk
    │   │   └── govuk-prototype-kit
    │   ├── dist
    │   │   ├── govuk
    │   │   ├── govuk-esm
    │   │   └── govuk-prototype-kit
    │   ├── package.json
    │   └── README.md
    └── govuk-frontend-review
        ├── src
        └── dist

srcpackages/govuk-frontend/src
packagepackages/govuk-frontendpackage/dist

To maintain backwards compatibility these directory changes aren't found in the published package.json

The problem with dist

Unfortunately the standard convention of building src to dist doesn't fit

We already reserve ./dist for our GitHub tagged releases, do we need to move it?
Will it be confusing to have a committed ./dist alongside ./package/src./package/dist build output?

Some options:

  1. dist — Leave it where it is
  2. distpackage/release — Maintain two build directories under ./package
  3. distpackage/dist/release — Consolidate single build directory under ./package

This PR has currently picked 1) from the list above

@colinrotherham colinrotherham requested a review from a team as a code owner April 4, 2023 13:20
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 4, 2023 13:21 Inactive
@colinrotherham colinrotherham linked an issue Apr 4, 2023 that may be closed by this pull request
5 tasks
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 4, 2023 13:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 6, 2023 08:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 6, 2023 10:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 6, 2023 13:54 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 09:03 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 13:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 14:01 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 14:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 15:19 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 16:08 Inactive
@colinrotherham colinrotherham changed the title [SPIKE] Performance: Review app imports govuk-frontend Performance: Review app imports govuk-frontend Apr 11, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 April 11, 2023 16:54 Inactive
@colinrotherham colinrotherham self-assigned this Apr 12, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 May 3, 2023 15:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 May 3, 2023 16:29 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 May 3, 2023 16:56 Inactive
src/README.md Outdated Show resolved Hide resolved
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 looks great - thanks for breaking up the PRs and keeping everything in sync, @colinrotherham, it's been really helpful.

Would appreciate another pair of eyes on this before merging, but from our discussion it didn't seem like there'd be any major objections.

Probably also a heads up to the team in Slack when this merges would be good, just to flag that folks are probably going to need to do some rebasing and such.

@domoscargin
Copy link
Contributor

Also worth popping a reminder here that this PR means we have to update our Decision on repo structure: alphagov/govuk-design-system-architecture#24

(I see @36degrees has just added that to the board as well)

This is in preparation for the `govuk-frontend` local workspace package to have access to source code, dependencies etc
This change temporarily links `govuk-frontend` to ./package/src via the wrapper package’s package.json `exports` field

For compatibility `npm publish` can continue from ./package/dist where the published package.json still exists with the current `exports` unchanged
In preparation for build artifacts being deleted, package will need utility tasks to run (on watch) during development
We ‘ve now split this task into 4x watchers:

1. Lint review app styles
2. Lint review app scripts
3. Build review app styles
4. Build review app scripts

With package getting its own tasks, watchers 1) and 2) no longer need to watch package files

Once the package build output has been removed from source, the review app will only need to watch package “dist” for changes not its entire source code
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3491 May 4, 2023 11:30 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.

Looks good to go for me as well, it's all super tidy ⛵ Thanks for keeping with all the feedback and rebases 😊

@romaricpascal
Copy link
Member

Agree with Brett on notifying people before we merge it, and making sure we don't wait too long before rebasing the PRs that still need rebasing as well 😊

@colinrotherham
Copy link
Contributor Author

colinrotherham commented May 4, 2023

Agree with Brett on notifying people before we merge it, and making sure we don't wait too long before rebasing the PRs that still need rebasing as well 😊

Thanks @romaricpascal

We rebased all these earlier:

cssnano-autoprefixer (#3243)
delete-package-dist (#3498)
remove-compat-mode-legacy-colours (#3576)
remove-compat-mode-legacy-fonts (#3574)
remove-ie8-css (#3572)
remove-ie8-js (#3570)
remove-typography-use-rem-setting (#3575)

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