-
Notifications
You must be signed in to change notification settings - Fork 323
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
Watch for changes to assets #3910
Conversation
Thanks @36degrees this is brilliant 🎉 For context, we spotted a list of things we didn't watch in this PR: But last week we added Gulp |
task.name('copy:assets watch', () => | ||
gulp.watch([ | ||
`${slash(options.srcPath)}/govuk/assets/` | ||
], assets(options)) |
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.
Not for this PR, but we might want to remove unlink
(delete) events from our watchers
], assets(options)) | |
], { events: ['add', 'change'] }, assets(options)) |
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 had noticed that when deleting a file from src
it wasn't deleted from package
, presumably because we're only running copy:files
and not clean
.
Removing the unlink
wouldn't fix that but would prevent running the task when a file's deleted when it's not going to do anything anyway?
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.
Yeah good point, it would only stop early triggering of tasks really—but might free up some "polling" CPU time for all our Windows users too?
Would be a separate issue to synchronise directories fully
Watch the `govuk/assets` folder and re-run the assets task when there are changes. This means that changes to the assets folder are applied to the dist folder (and therefore are reflected in the review app) without having to manually run the `build:package` task.
@@ -16,19 +16,12 @@ export default (options) => gulp.series( | |||
files.clean('*', options) | |||
), | |||
|
|||
assets(options), |
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.
Should we share this new task with release.mjs
too?
It's mostly identical between package.mjs
and release.mjs
Would just need destPath
overriding without the govuk/
prefix
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.
Will leave this as a future improvement if that's alright.
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.
Approved! One minor comment about reusing tasks but not a blocker
Watch for changes to assets
I was taking a look at #3873 and noticed that changes to assets were not reflected in the review app unless I manually ran
npm run build:package
.Watch the
govuk/assets
folder and re-run the assets task when there are changes.This means that changes to the assets folder are applied to the dist folder (and therefore are reflected in the review app) without having to manually run the
build:package
task.