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.

Sign up for GitHub

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

Ensure npm scripts (via Gulp) emit errors #3033

Merged
merged 2 commits into from
Nov 28, 2022
Merged

Ensure npm scripts (via Gulp) emit errors #3033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83f8806
Don’t log npm script `--silent` flag
colinrotherham Nov 23, 2022
e3665b4
Ensure npm scripts (via Gulp) emit errors
colinrotherham Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions gulpfile.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { npmScriptTask } from './tasks/run.mjs'
* Runs JavaScript code quality checks, documentation, compilation
*/
gulp.task('scripts', gulp.series(
npmScriptTask('lint:js', ['--silent']),
npmScriptTask('build:jsdoc', ['--silent']),
npmScriptTask('lint:js'),
npmScriptTask('build:jsdoc'),
compileJavaScripts
))

Expand All @@ -24,8 +24,8 @@ gulp.task('scripts', gulp.series(
* Runs Sass code quality checks, documentation, compilation
*/
gulp.task('styles', gulp.series(
npmScriptTask('lint:scss', ['--silent']),
npmScriptTask('build:sassdoc', ['--silent']),
npmScriptTask('lint:scss'),
npmScriptTask('build:sassdoc'),
compileStylesheets
))

Expand All @@ -36,8 +36,8 @@ gulp.task('styles', gulp.series(
gulp.task('compile', gulp.series(
compileJavaScripts,
compileStylesheets,
npmScriptTask('build:jsdoc', ['--silent']),
npmScriptTask('build:sassdoc', ['--silent'])
npmScriptTask('build:jsdoc'),
npmScriptTask('build:sassdoc')
))

/**
Expand All @@ -48,7 +48,7 @@ gulp.task('dev', gulp.series(
clean,
'compile',
watch,
npmScriptTask('serve', ['--silent', '--workspace', 'app'])
npmScriptTask('serve', ['--workspace', 'app'])
))

/**
Expand Down
27 changes: 23 additions & 4 deletions tasks/run.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,35 @@ export async function npmScript (name, args = []) {
const command = process.platform === 'win32' ? 'npm.cmd' : 'npm'

return new Promise((resolve, reject) => {
const script = spawn(command, ['run', name, ...args])
const script = spawn(command, ['run', name, '--silent', ...args])
Copy link
Contributor Author

@colinrotherham colinrotherham Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still add --silent behind the scenes because we don't want both npm AND Gulp logging progress to the console—one CLI tool is enough

Note: Output from the npm task (linting for example) is not silenced


// Send output to console
script.stdout.on('data', (data) => console.log(data.toString()))
script.stderr.on('data', (data) => console.error(data.toString()))

// Emit errors to error listener
script.stderr.on('data', (data) => {
script.emit('error', new Error(data.toString()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By emitting an error here it ensures we hit reject() (below) via the error listener

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Won't that trigger multiple times if a script outputs to stderr multiple times? Furthermore, I'm worried:

  • it'll catch false errors for a script that just outputs to stderr as a convenient second buffer. I thought the actual marker of a script erroring was a non-zero exit (covered by the close)
  • it'll double up with the close handling later

question: Why are we emitting an error rather than directly rejecting? That feels an unnecessary extra step there.

Copy link
Contributor Author

@colinrotherham colinrotherham Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's confusing, probably why we wanted to remove Gulp 😭

Gulp sources are Node.js Stream instances (or early "stream" implementations that our Gulp plugins use)

They extend Node.js EventEmitter so we're going to follow the approach of expecting our tasks to emit:

task.emit('end') // Gulp piped task has finished processing
stream.emit('error') // Gulp stream has encountered an error

If we only reject() the promise we miss all the lovely error messages (lint output, line numbers etc)

By emitting events we can ensure the Gulp runner stays in control (logs the error and exits) but it also lets us listen via .on('error') and alternatively NOT exit (log only) like during gulp dev 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't that trigger multiple times if a script outputs to stderr multiple times?

The first error written will hit script.on('error', reject) so you'll only see the first one

})

// Reject on actual script errors to exit `gulp dev`
script.on('error', reject)

// Resolve all exit codes to continue `gulp dev`
script.on('close', resolve)
// Check for exit codes or continue `gulp dev`
script.on('close', (code) => {
let error

// Closed with errors
if (code > 0) {
error = new Error(`Task for npm script '${name}' exit code ${code}`)

// Hide error info (already written to `stderr`)
error.showProperties = false
error.showStack = false
}

// Reject on errors
error ? reject(error) : resolve(code)
})
})
}

Expand Down