Build: `onError` event not triggered with `utils.build.failPlugin()`

[tzmanics]

Describe the bug
I expected to have onError event triggered as long as there was an error.

Talking to @ehmicky it currently has this behavior:

I was expecting the onError to only be triggered when the build fails, and since failPlugin() does not make the build fail, it should not trigger onError

I believe this may add confusion or inconsistencies if an error that exists doesn't trigger onError if utils.build.failPlugin is used.

Here is what this looks like in the build logs:

[ehmicky]

I think if this is what you expected to happen, other users might expect it too, so we should consider triggering onError on failPlugin(). I would be curious what @verythorough or @erquhart thinks too?

Some pending questions I would have:

• At the moment, onError is guaranteed to happen at the end of the build (right before onEnd). I am assuming this would be the case here too, i.e. it would still wait for the build to complete to trigger onError? This would also keep it consistent with onEnd and onSuccess.
• At the moment, onError is only triggered once and it is passed an error as argument. To do the above, we would need to either call onError once per failPlugin(), or to pass an array of errors instead as argument. What are your thoughts on this?

[ehmicky]

Fixed by #1254:

• onError is triggered by failPlugin()
• onError is still guaranteed to happen at the end of the build
• onError is passed the last error instance

[verythorough]

I wonder if this is a naming issue. Are there any other non-fatal errors that trigger onError, or is it only triggered when the build actually fails?

If the latter, it sounds like the hook should be called onFailure (which matches onSuccess), and it should _not_ include plugin failures that don't fail the build. (And then perhaps we consider adding some sort of onPluginFailure and/or onError events.)

[ehmicky]

I think there could be two things plugin authors might want to handle:

• build failing
• error in the plugin itself

At the moment (since #1254), onError is triggered on both. Following your line of thoughts, this could make sense to distinguish between the two, e.g. onBuildFailure vs onPluginFailure.

Note that this would not be a completely trivial change as it might need a rework of how we orchestrate plugins. Also, any renaming requires some work to maintain backward compatibility.

Edit: some additional explanation on the current logic. (see #1774).
If a plugin throws an uncaught exception, uses utils.build.failBuild() or uses utils.build.cancelBuild():

• the build and the plugin fail
• all events for all plugins are skipped, except onError and onEnd
• the onError events for all plugins are run

If a plugin uses utils.build.failPlugin():

• the build does not fail, but the plugin does
• all events for the current plugin are skipped, except onError and onEnd
• the onError event for the current plugin is run
• other plugins are not impacted

onError() is not run otherwise.
onEnd() is always run, regardless of whether the current or other plugins failed.

Edit 2: the new onSuccess behavior introduces the following changes:

• errors inside onSuccess and onEnd do not make the build fail nor trigger onError

[ehmicky]

This behavior has been a source of confusion. I would suggest the following solution as a way to improve the current behavior:

• onError should only mean "on build failure", not "on plugin failure". This means it would be triggered if any Build plugin fails before deploy. This implies errors in onSuccess and onEnd would not trigger onError.
• "on plugin failure" should be implemented by plugins using try/catch blocks instead. IMHO having to fallback to a generic onError callback is the wrong solution to the right problem. Instead, Build plugins should wrap any code that might fail with try/catch blocks. If they wish to cleanup resources, onEnd can be used. If it turns out try/catch is not good enough for some plugin authors, we can always add a new onPluginFailure event in the future.

This should make documenting onError much easier as the event would then clearly be happening after onPostBuild and before onSuccess.

Also, that change would be pretty lightweight to implement and not require any plugins migration.

What do you think?

[rstavchansky]

@ehmicky This would certainly make the documentation task easier for onError. I'd want to make sure that we include a tip or something in our docs to encourage authors of Build plugins to wrap any code that might fail with try/catch blocks.

[verythorough]

@ehmicky I think that makes total sense. Also, when something is easier to document, it usually means it's easier to use!

[rstavchansky]

@ehmicky Can you confirm the timeline for this change?

_If_ this is something that will get implemented in the next week or so, I plan to address the docs changes for this in this related, open docs PR: https://github.com/netlify/docs/pull/722.

_If_ this will sit for a while, while the other build plugin event handler changes go out, please let me know, and I'll update the docs in a separate PR at a later date.

[ehmicky]

This is something that I can implement in the first few days of next week, providing you all think this is a good idea. :+1:

[ehmicky]

Summarizing the new behavior.

In a nutshell

onError, onSuccess and onEnd mean "on build error/success/end", not "on plugin error/success/end". They always happen in that order.

Plugins should rely on try/catch/finally for exceptions handling.

Details

A plugin fails _without making the build fail_ when either:

• using utils.build.failPlugin()
• the failure happens after deploy. This means: inside any onError, onSuccess or onEnd event handler.

When that happens, no other events for that specific plugin is run. Not even onError and onEnd. Plugins should use try/catch/finally blocks to handle exceptions, not onError nor onEnd.

Otherwise, most failures will make the build fail. This includes:

• the build command failed
• Functions or Edge handlers bundling failed
• the deploy failed (deploying files to our CDN)
• a plugin onPreBuild, onBuild or onPostBuild event handler failed. This includes uncaught exceptions and using utils.build.failBuild() or utils.build.cancelBuild().

onError is triggered when the build failed. It means "on build failure" not "on plugin failure".
onSuccess is the opposite. It is triggered after the build succeeded.
onEnd is triggered after the build either failed or succeeded. It is useful for resources cleanup.

[ehmicky]

PRs at #1884 and #1885.