Catch possible errors in listeners to avoid breaking execution

[avillegasn]

Description

Imagine we have two plugins (or code fragments) that subscribe to changes in the block editor. The first listener function subscribed is:

wp.data.subscribe( () => {
  console.log( 'Hey!' );
  let a; const b = a.b; // Error: Cannot read property ‘b’ of undefined
} );

and the second listener function subscribed is:

wp.data.subscribe( () => {
  console.log( 'S.O.S' ); // This log won't go out because of others breaking execution
} );

An error thrown in the first listener will prevent the execution of the second listener when a change in the editor triggers the execution of all listeners subscribed to changes. This may cause careless programmers to break the execution of others' code.

The proposed solution here surrounds the execution of each listener with a try/catch statement that avoids the execution break.

Some may say that code inside listeners should be developed in a way to avoid throwing errors. But adding the try/catch we make sure this will not happen.

How has this been tested?

Just copy the previous pair of listeners, paste them in the web browser JS console when editing a post in the block editor, and make some change to trigger their execution.

Types of changes

I added a try/catch statement with a console.log printing the error.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[aduth]

I'm conflicted on this. On one hand, we have some precedent in preventing plugins from crashing the editor with block-level and application-level error boundaries. On the other hand, I'm not sure we want or need to have try / catch in every extension point, or at least not without some protocol for when and how it be introduced (why here and not in some other extension point?).

I guess it depends:

• Do these errors crash the editor?
• Are the other ways we could isolate the subscribers to avoid an error affecting the next?

[avillegasn]

Thanks for the comment @aduth. These errors do not crash the editor, but may crash others code. I don't have enough expertise to know other ways to isolate the listener functions. I've been exploring this for a while and didn't find a better solution than the try/catch approach.

Again, not sure if other extension points also need this kind of solutions, but it was really annoying find this problem I mention in the PR the hard way (one of our users complained about our plugin not working when in fact the error was another plugin breaking the subscription process by subscribing a listener function with errors...).

As a plugin developer, if an apparently innocuous solution like the try/catch presented here may help avoid these kind of problems caused by unrespectful developers, I'd totally go for it.

On the other hand, I understand your concerns.

[aduth]

It's an interesting problem. I'd agree that ultimately the current situation leads to scenarios like the one you describe, which are very difficult to debug. Since the default behavior of an uncaught thrown error is to halt iteration, I'd also agree that a try / catch would prove beneficial here to provide the insulation between callbacks. The main uncertainty I have then is... what do we do with the error in the catch ? Is console.error as you've proposed more-or-less equivalent to what a browser does with an uncaught error? Should we be doing more in the application to not allow errors to go uncaught?

The last question is more of a broad point, but it ties back to my previous comment on consistent approaches to where and how we handle errors in the application.

As it impacts this pull request, I might suggest considering one the following actions:

• We revise the implementation to "hold" errors until the end, then throw them intentionally (example). The only upside to this is that we don't allow iteration to halt, while retaining the behavior to consistently not handle errors. The downsides being: It's more complex, and uncaught errors are still a bad thing.
• We keep it as you've proposed here, under consideration that console.error is a good enough way to handle the thrown error.

I might also consider raising this as a discussion point for the upcoming JavaScript meeting tomorrow.

[avillegasn]

Thanks for the comments @aduth. I'm not sure at all about what a browser does with an uncaught error, but when I was thinking about this problem and trying to find solutions to it I was reading several sources on how to handle errors and almost everybody was using console.error. You can see examples here, here, and here.

Then I got to the same approach you linked 'holding' errors and throwing them afterwards. However, I found the console.error approach more simple and elegant.

I believe the feedback from the JavaScript meeting later today may help here.

[youknowriad]

Hi There! I've triaging PRs today and it seems this one is stale. how was the discussion in #core-js
I'm going to close now but please consider reopening if there were some concrete action items to move this forward. Thanks for your efforts.