-
Notifications
You must be signed in to change notification settings - Fork 334
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
Conversation
9c54e4a
to
35404c5
Compare
35404c5
to
9406f38
Compare
9406f38
to
ae8048e
Compare
ae8048e
to
06bb76f
Compare
06bb76f
to
232b3b2
Compare
232b3b2
to
11d483e
Compare
11d483e
to
7b1bcdd
Compare
7b1bcdd
to
15ddf8d
Compare
15ddf8d
to
fa6e7e7
Compare
fa6e7e7
to
4aeb4d7
Compare
govuk-frontend
govuk-frontend
4aeb4d7
to
aff5e63
Compare
aff5e63
to
681bdbf
Compare
b3de34b
to
48e5ab3
Compare
48e5ab3
to
2c2a9d9
Compare
There was a problem hiding this 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.
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
Moves `govuk-frontend` and the Review app packages into “packages” https://docs.npmjs.com/cli/v9/using-npm/workspaces#defining-workspaces
2c2a9d9
to
6d9eb1a
Compare
There was a problem hiding this 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 😊
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) |
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
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 bundlePackage directory changes
Previously
npm link
only resolved the pre-built packages files but they're now consolidated:Before
After
→src
packages/govuk-frontend/src
→package
packages/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
todist
doesn't fitWe 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:
dist
— Leave it where it is→dist
package/release
— Maintain two build directories under./package
→dist
package/dist/release
— Consolidate single build directory under./package
This PR has currently picked 1) from the list above