-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: emit system log when plugin has failed to load #5679
Conversation
This pull request adds or modifies JavaScript ( |
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.
Approach looks good and the test gives confidence. Leaving some questions.
t.is(systemLog.trim(), 'An error message thrown by Node.js') | ||
} | ||
|
||
t.snapshot(normalizeOutput(output)) |
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.
Could we replace the snapshot with a more precise assertion?
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.
The snapshot is just an accessory thing to ensure we're not throwing an unexpected error somewhere else. The main assertion is ensuring the stuff we pushed to stderr is piped to system logs:
build/packages/build/tests/plugins/tests.js
Line 364 in 59fe711
t.is(systemLog.trim(), 'An error message thrown by Node.js') |
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.
If it's about asserting there's no error, maybe it's better to check something like build success? We're always complaining about the snapshot tests so much, so I think we shouldn't add them unnecessarily.
packages/build/src/plugins/load.js
Outdated
const bufferedStdErr = [] | ||
|
||
if (featureFlags.netlify_build_plugin_system_log && childProcess.stderr) { | ||
childProcess.stderr.on('data', (data) => { |
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 unregister this after we're done initialising?
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.
Good point. Done in a0b4327.
Summary
Right now we don't have any visibility over errors that occur when loading a plugin. This PR tries to address that by emitting system logs for any failures that happen during the plugin loading stage.