Separate out CSS/JS lint tasks from `copy-assets` #2863

colinrotherham · 2022-09-20T08:27:38Z

This PR does a few things:

Renames gulp copy-assets → gulp compile for clarity
Renames gulp copy-files to gulp copy:files for consistency
Separates out CSS/JS lint tasks from build and test
Tackles the ./contributing/tasks.md documentation

Changes to running npm scripts

For npm scripts in particular I've updated runTask() → npmScriptTask() to work with Gulp

gulp.task('scripts', gulp.series(
  npmScriptTask('lint:js', ['--silent']),
  'js:compile'
))

Which keeps friendly task names formatted for the CLI too

[12:54:34] Using gulpfile ~/path/to/project/govuk-frontend/gulpfile.js
[12:54:34] Starting 'scripts'...
[12:54:34] Starting 'npm run lint:js --silent'...
[12:54:35] Finished 'npm run lint:js --silent' after 1.7 s
[12:54:35] Starting 'js:compile'...
[12:54:37] Finished 'js:compile' after 2.03 s
[12:54:37] Finished 'scripts' after 3.73 s

When do linting tasks now run?

Linting tasks don't run before tests
Linting tasks don't run before build
Linting tasks do run during gulp dev via watch task
Linting tasks do run during PR checks via GitHub workflow

Open for feedback of course. Pairs up nicely with #2862 for a bit of a speed increase and closes #2855

domoscargin · 2022-09-21T07:26:17Z

docs/contributing/tasks.md

 - run accessibility tests on HTML files
 - run tests on the review application

 **`npm run heroku` runs on Heroku build/PR and it:**
 - compiles components' HTML
- compiles CSS & JS
+- compiles Sass & JavaScript


This is very understandable, but it would be more accurate to say "compiles Sass to CSS". That's probably needlessly wordy, though?

Thanks for this @domoscargin

The diff has changed but I've gone with:

Short descriptions

For npm scripts (or gulp tasks with sub tasks)

**`npm run heroku` runs on Heroku build/PR and it will:** - run `npm run build:compile` - start up Express

Long descriptions

For the main gulp task that does the work

**`gulp styles`** This task will: - check Sass code quality via Stylelint (`npm run lint:scss`) - compile Sass to CSS (`gulp scss:compile`) into `./public`, or another location via the `--destination` flag **`gulp scripts`** This task will: - check JavaScript code quality via ESLint (`npm run lint:js`) (using JavaScript Standard Style) - compile JavaScript ESM to CommonJS (`gulp js:compile`) into `./public`, or another location via the `--destination` flag

Is that alright?

domoscargin · 2022-09-21T07:28:13Z

docs/contributing/tasks.md

@@ -70,18 +65,21 @@ This task will:
 **`gulp styles`**

 This task will:


"This task performs:"?

Or add a verb to the beginning of each bullet point.

I've added verbs to all bullets, thanks 😊

domoscargin · 2022-09-21T07:28:50Z

docs/contributing/tasks.md

- This task will:
- - concatenate and uglify javascript (`gulp js:compile`) to a destination folder that can be specified via a --destination flag
+This task will:
+ - JavaScript code quality checks via ESLint (`gulp js:lint`) (using JavaScript Standard Style)


Needs a verb to start

domoscargin · 2022-09-21T07:32:09Z

docs/examples/webpack/assets/main.js

@@ -1,3 +1,7 @@
-import Button from 'govuk-frontend/govuk/components/button/button'


Could we split this commit out?

Or add something to the commit message about why it's part of this linting PR.

I've moved the commit to a separate PR, bit simpler that way #2869

domoscargin · 2022-09-21T07:38:38Z

gulpfile.js

-// --------------------------------------
+/**
+ * Serve task
+ * Restarts Node.js app when there are changes


Could we reflect that Sass changes cause a rebuild (via the watch task)?

I've added more detail to the watch task for .scss and .mjs files

This one is serve (via Nodemon) so will only restart for Node.js related changes

domoscargin · 2022-09-21T07:39:03Z

gulpfile.js

-// --------------------------------------
+/**
+ * Dev task
+ * Runs a sequence of task on start


Suggested change

* Runs a sequence of task on start

* Runs a sequence of tasks on start

Thanks, fixed

domoscargin · 2022-09-21T07:49:28Z

gulpfile.js

+/**
+ * Copy assets task
+ * Copies assets to taskArguments.destination (public)
+ */
 gulp.task('copy:assets', () => {


We're referring to two things as "assets" in these tasks:

The static assets in src/assets

The compiled JS and CSS files

Maybe we should rename this to copy:static-assets or something.

Yep I noticed this too!

Hope the change I went with clears things up?

Rename gulp copy-assets → gulp compile (compiles CSS, Sass, Sassdoc)
Rename gulp copy-files → gulp copy:files (compiles selected files into --destination)

Gulp also looks for .displayName properties (see Task metadata) on plain Node.js functions so I've added these so everything makes sense from the command line:

clean:dist clean:package clean:public

Oh and as a bonus, it'll now output the actual npm script name too (not our task wrappers)

[12:27:26] Using gulpfile ~/path/to/project/govuk-frontend/gulpfile.js [12:27:26] Starting 'styles'... [12:27:26] Starting 'npm run lint:scss --silent'... [12:27:29] Finished 'npm run lint:scss --silent' after 3.68 s [12:27:29] Starting 'scss:compile'... [12:27:30] Finished 'scss:compile' after 409 ms [12:27:30] Finished 'styles' after 4.09 s

domoscargin · 2022-09-21T08:14:09Z

I've left a few small comments, but overall I think this looks good.

It'd be good to update this sheet when this gets approved: https://docs.google.com/spreadsheets/d/1aprdKVIx6FzBtX9ohWx6LR8p5XH3s2PNq1tev1ZwxkY/edit#gid=77403370

I can't think of any situation where we'd be hindered by not running linting before builds. Our Github workflow would catch anything getting pushed, and our dev/start task would catch anything we were working on locally.

I'll leave this open to see if anybody else wants to chime in.

romaricpascal

I've added a few naming comments as I was trying to wrap my head around what runTask was doing, but otherwise it looks neat to me 😄

I think we'll need some further work around linting and testing though. There's a scenario that's pretty annoying when working on tests running with Puppeteer.

if you update a non-test JS file with a linting error,
the linting still runs on the gulp dev that runs to recompile, so nothing will be re-compiled
your test will keep running on the old version of the code, and likely still be failing
and, that's the annoying bit, the info on why it fails is in another terminal than the one running your test and that's tricky to see.

I'm not sure how we'd feel about removing the linting from the dev task altogether and just having it run on CI? Perhaps something to be done part of adding Git hooks to ensure there's still some local checks before code reaches Github.

romaricpascal · 2022-09-26T11:09:12Z

tasks/run.js

+const runTask = (name, args = []) => {
+  const script = `${name} ${args.join(' ')}`.trim()
+
+  // Create named runner
+  // https://gulpjs.com/docs/en/api/task#task-metadata
+  const runner = () => task(name, args)
+  runner.displayName = `npm run ${script}`
+
+  return runner
+}


I'm a bit confused by the naming. From what I understand, this'll only run npm scripts, so maybe we should clarify this by renaming to runNpmScript ?

Mind if I use the following?

Function for npm script
task() → npmScript()

Function for gulp task (that returns npm script)
runTask() → npmScriptTask()

romaricpascal · 2022-09-26T11:13:37Z

tasks/run.js

+
+  // Create named runner
+  // https://gulpjs.com/docs/en/api/task#task-metadata
+  const runner = () => task(name, args)


I think there's a bit of naming collision there as this is not the task function from Gulp, but our own from right above. We may want to rename that taskForNpmScript for clarification, especially as it doesn't have the same signature as Gulp's task.

Since Gulp v4 tasks can be "a stream, promise, event emitter, child process, or observable"

I've flipped it the other way around (function that returns Promise<number>)

Suggested change

const runner = () => task(name, args)

const task = () => npmScript(name, args)

romaricpascal · 2022-09-26T11:17:08Z

tasks/run.js

+ * @returns {() => Promise<number>} Exit code
+ */
+const runTask = (name, args = []) => {
+  const script = `${name} ${args.join(' ')}`.trim()


Maybe slightly nitpicky, but it seems that line is only used to generate the displayName. I got confused by why it was computed early, but then not passed to task down below, so I think it'd flow more clearly if it was right before the runner.displayName call.

That's fine, can put it all on the same line

// Add task alias // https://gulpjs.com/docs/en/api/task#task-metadata task.displayName = `npm run ${name} ${args.join(' ')}`.trim()

Improves compatibility on Windows/Linux/Mac for linting tools

Also includes `configPaths` consistency and JSDoc descriptions

Also moves sassdoc into `gulp compile` sub tasks

Linting tasks no longer run before assets are built or tested

See https://gulpjs.com/docs/en/api/task#task-metadata

colinrotherham · 2022-09-27T09:00:07Z

@romaricpascal Mind taking another look?

Can you give me an example of an ESLint error that blocks the JavaScript compile too?

I tried a simple one here showing how we resolve (not reject) the npm script Promise for all exit codes

[09:48:48] Starting 'scripts'...
[09:48:48] Starting 'npm run lint:js --silent'...
Server started at http://localhost:3000

/path/to/project/govuk-frontend/src/govuk/components/accordion/accordion.mjs
  36:71  error  Extra semicolon  semi

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.


[09:48:49] Finished 'npm run lint:js --silent' after 1.28 s
[09:48:49] Starting 'js:compile'...
[09:48:51] Finished 'js:compile' after 1.64 s
[09:48:51] Finished 'scripts' after 2.92 s

romaricpascal

Can you give me an example of an ESLint error that blocks the JavaScript compile too?

I tried a simple one here showing how we resolve (not reject) the npm script Promise for all exit codes

Oh my bad, I missed that, sorry 😞 . Now it does work nicely with the scenario I described before 🙌🏻 Looks good to go for me 😄

colinrotherham · 2022-10-13T12:02:44Z

This PR was related to #2243

Missed from #2863

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 08:27 Inactive

colinrotherham added tooling javascript squad labels Sep 20, 2022

colinrotherham force-pushed the upgrade-test-jest-percy branch from f3146cd to 5e550d8 Compare September 20, 2022 08:58

colinrotherham requested a review from a team as a code owner September 20, 2022 08:58

Base automatically changed from upgrade-test-jest-percy to main September 20, 2022 09:11

colinrotherham changed the title ~~Linting tasks no longer run before copy-assets~~ Separate out CSS/JS lint tasks from copy-assets Sep 20, 2022

colinrotherham mentioned this pull request Sep 20, 2022

Stop linting errors from stopping JS being compiled for tests #2855

Closed

colinrotherham force-pushed the lint-separately branch from 40f87ea to 3fc590e Compare September 20, 2022 10:49

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 10:49 Inactive

colinrotherham force-pushed the lint-separately branch from 3fc590e to 9ebc415 Compare September 20, 2022 11:05

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 11:06 Inactive

colinrotherham force-pushed the lint-separately branch from 9ebc415 to 90f427f Compare September 20, 2022 18:19

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 18:19 Inactive

colinrotherham force-pushed the lint-separately branch from 90f427f to c4a34eb Compare September 20, 2022 19:04

colinrotherham changed the base branch from main to gulp-completion September 20, 2022 19:05

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 19:05 Inactive

colinrotherham force-pushed the lint-separately branch from c4a34eb to 92233ac Compare September 20, 2022 19:11

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 19:11 Inactive

colinrotherham force-pushed the gulp-completion branch from 1164290 to 6559896 Compare September 20, 2022 20:04

colinrotherham force-pushed the lint-separately branch from 92233ac to 3b31bfd Compare September 20, 2022 20:05

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 20, 2022 20:06 Inactive

domoscargin reviewed Sep 21, 2022

View reviewed changes

colinrotherham mentioned this pull request Sep 21, 2022

Fix webpack docs example #2869

Merged

colinrotherham force-pushed the gulp-completion branch 3 times, most recently from f8b2827 to 0b54976 Compare September 21, 2022 12:18

Base automatically changed from gulp-completion to main September 21, 2022 12:22

colinrotherham force-pushed the lint-separately branch from 00907ff to b669f11 Compare September 22, 2022 11:21

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 22, 2022 11:21 Inactive

colinrotherham force-pushed the lint-separately branch from b669f11 to 0dddc0d Compare September 22, 2022 11:46

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 22, 2022 11:46 Inactive

colinrotherham force-pushed the lint-separately branch from 0dddc0d to 5ae1389 Compare September 22, 2022 11:48

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 22, 2022 11:48 Inactive

colinrotherham requested a review from domoscargin September 22, 2022 11:58

colinrotherham force-pushed the lint-separately branch from 5ae1389 to f37d032 Compare September 26, 2022 11:26

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 26, 2022 11:26 Inactive

romaricpascal reviewed Sep 26, 2022

View reviewed changes

colinrotherham added 2 commits September 26, 2022 14:28

Ensure git clone uses LF line endings

dfe47a5

Improves compatibility on Windows/Linux/Mac for linting tools

Review documentation for gulp tasks

30dd184

Also includes `configPaths` consistency and JSDoc descriptions

colinrotherham force-pushed the lint-separately branch from f37d032 to 3a9d281 Compare September 26, 2022 13:30

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 26, 2022 13:31 Inactive

colinrotherham added 7 commits September 27, 2022 09:37

Add npm task wrapper for gulp

d207180

Use npm wrapper to replace gulp lint tasks

e6fd817

Rename gulp copy-assets to gulp compile

8e91900

Also moves sassdoc into `gulp compile` sub tasks

Rename gulp copy-files to gulp copy:files

e4f4b1b

Rename gulp copy-files to gulp copy:files

83f9106

Separate out CSS/JS lint tasks from build and test

4c11f65

Linting tasks no longer run before assets are built or tested

Add display names to task functions

10d4581

See https://gulpjs.com/docs/en/api/task#task-metadata

colinrotherham force-pushed the lint-separately branch from 3a9d281 to 10d4581 Compare September 27, 2022 08:56

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2863 September 27, 2022 08:56 Inactive

romaricpascal approved these changes Sep 27, 2022

View reviewed changes

colinrotherham merged commit 8f370c2 into main Sep 27, 2022

colinrotherham deleted the lint-separately branch September 27, 2022 11:46

colinrotherham added a commit that referenced this pull request Nov 1, 2022

Clarify linting docs now separate to npm test

2bcd44a

Missed from #2863

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate out CSS/JS lint tasks from `copy-assets` #2863

Separate out CSS/JS lint tasks from `copy-assets` #2863

colinrotherham commented Sep 20, 2022 •

edited

Loading

domoscargin Sep 21, 2022

colinrotherham Sep 22, 2022

domoscargin Sep 21, 2022 •

edited

Loading

colinrotherham Sep 22, 2022

domoscargin Sep 21, 2022

domoscargin Sep 21, 2022

colinrotherham Sep 21, 2022

domoscargin Sep 21, 2022

colinrotherham Sep 22, 2022

domoscargin Sep 21, 2022

colinrotherham Sep 22, 2022

domoscargin Sep 21, 2022

colinrotherham Sep 22, 2022

domoscargin commented Sep 21, 2022 •

edited

Loading

romaricpascal left a comment

romaricpascal Sep 26, 2022

colinrotherham Sep 27, 2022

romaricpascal Sep 26, 2022

colinrotherham Sep 27, 2022

romaricpascal Sep 26, 2022

colinrotherham Sep 27, 2022

colinrotherham commented Sep 27, 2022

romaricpascal left a comment

colinrotherham commented Oct 13, 2022

		@@ -70,18 +65,21 @@ This task will:
		`gulp styles`

		This task will:

		@@ -1,3 +1,7 @@
		import Button from 'govuk-frontend/govuk/components/button/button'

	* Runs a sequence of task on start
	* Runs a sequence of tasks on start

	const runner = () => task(name, args)
	const task = () => npmScript(name, args)

Separate out CSS/JS lint tasks from copy-assets #2863

Separate out CSS/JS lint tasks from copy-assets #2863

Conversation

colinrotherham commented Sep 20, 2022 • edited Loading

Changes to running npm scripts

When do linting tasks now run?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Short descriptions

Long descriptions

domoscargin Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoscargin commented Sep 21, 2022 • edited Loading

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Sep 27, 2022

romaricpascal left a comment

Choose a reason for hiding this comment

colinrotherham commented Oct 13, 2022

Separate out CSS/JS lint tasks from `copy-assets` #2863

Separate out CSS/JS lint tasks from `copy-assets` #2863

colinrotherham commented Sep 20, 2022 •

edited

Loading

domoscargin Sep 21, 2022 •

edited

Loading

domoscargin commented Sep 21, 2022 •

edited

Loading