-
Notifications
You must be signed in to change notification settings - Fork 331
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
Build tasks: Move review app tasks into app/tasks
#3384
Conversation
2da619b
to
cd0e06b
Compare
49b1279
to
62da944
Compare
cd0e06b
to
4634755
Compare
62da944
to
0354637
Compare
4634755
to
914e09a
Compare
0354637
to
d48f97f
Compare
914e09a
to
7d12dce
Compare
d48f97f
to
2dcb55d
Compare
7d12dce
to
259203b
Compare
2dcb55d
to
bfa6e52
Compare
259203b
to
bd7a622
Compare
bfa6e52
to
3da41a9
Compare
bd7a622
to
c74785a
Compare
3da41a9
to
080347e
Compare
c74785a
to
28db545
Compare
080347e
to
6da759c
Compare
28db545
to
16ff2e8
Compare
ebc21bf
to
d761c8c
Compare
d761c8c
to
d970378
Compare
d970378
to
7b5fc1e
Compare
7b5fc1e
to
a47ebdb
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 it's almost there, just had a couple of questions and little things in the comments 😄 Cheers for the move to plain functions with task.name
and the genral tidy up.
@@ -20,18 +20,16 @@ export function watch () { | |||
`${slash(paths.src)}/govuk/**/*.scss`, | |||
`!${slash(paths.src)}/govuk/vendor/*` | |||
], gulp.parallel( | |||
npm.run('lint:scss'), | |||
npm.script('lint:scss'), |
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 the naming works, run
makes it clear that something will happen while script
sounds more like something you'll reference in a variable for later 🙌🏻
@@ -4,7 +4,8 @@ const { ports } = require('./config/index.js') | |||
* @type {import('jest-dev-server').Config} | |||
*/ | |||
module.exports = { | |||
command: 'npm start --workspace app', | |||
// Start Express.js without "prestart" build script | |||
command: 'npm start --ignore-scripts --workspace app', |
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.
question: Wouldn't running the prestart
build script guarantee that our tests run against the last build of the server? Is it run some other way?
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.
This is a learning from the GOV.UK Design System
https://github.com/alphagov/govuk-design-system/blob/main/jest-puppeteer.config.js#L6
Every time you run npm test
or npx jest ./path/to/spec.test.js
it does a full rebuild each time
So here we cheat and give Jest permission to run what's already built
For anyone else running npm start
I'd like it to stay nice and simple and build first 😊
Well spotted though
"serve": "nodemon", | ||
"prestart": "npm run build", |
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.
nitpick As we're looking at reducing the complexity of our build, I think I'd be keen to avoid that prestart
step that magically links the build to all our start
calls. It also allows to keep the build and the start of the app separate without having to know that you need to reach for --ignore-scripts
(which is a bit arcane).
I think it'll only be used on heroku so could we maybe make things explicit in the Procfile with something like (cd app && run build && npm start)
(or with two --workspace app
on each command).
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.
Would you mind if we kept npm start
?
Keeps things simple
Although perhaps in future should align start
/dev
?
Server start
npm start
npm start --workspace app
npm start --workspace docs/examples/webpack
Development
npm run dev
npm run dev --workspace app
npm run dev --workspace docs/examples/webpack
But for now we have a necessary mismatch with npm start
at project-level running this:
npm run dev --workspace app
.github/workflows/screenshots.yml
Outdated
@@ -41,7 +41,7 @@ jobs: | |||
uses: ./.github/workflows/actions/build | |||
|
|||
- name: Start review application | |||
run: npm run serve & | |||
run: npm start --workspace app & |
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.
Need to make sure we skip the prestart
build here (as it's cached in GitHub Actions)
run: npm start --workspace app & | |
# Start Express.js without "prestart" build script | |
run: npm start --ignore-scripts --workspace app & |
a47ebdb
to
3891d17
Compare
3891d17
to
7887cab
Compare
Only Node.js review app `*.mjs` files should restart the server
Used name a task function for the Gulp CLI, but defer it until run
Whilst Gulp is still used as a task runner, I've switched to the “export your tasks” pattern which lets us remove task “wrapper” functions: https://gulpjs.com/docs/en/getting-started/creating-tasks
7887cab
to
69b3f08
Compare
This PR splits out all "Review app" tasks into
app/tasks
as part of:This reduces the risk of affecting
govuk-frontend
output by keeping each build separate:./tasks
via gulpfile.mjs./app/tasks
via app/gulpfile.mjsCreating tasks
Whilst Gulp is still used as a task runner, I've switched to the export your tasks pattern vs using
gulp.task()
All our new tasks are now plain async functions:
But they can be composed into tasks for Gulp's
gulp.series()
andgulp.parallel()
etc