(Do not) suppress errors by default

[rwojsznis]

Re-issued by @sstephenson's request. Related comment in #53

Summary:

Currently javascript errors are not being propagated by default. Custom error handler console-warns about errors - it breaks default behavior of popular error catcher services like sentry that are relying on window.onerror function.

I don't think such behavior is developer friendly and expected for outside (Basecamp) developers - may I ask what was the reasoning behind it? 🤔

[sstephenson]

Thanks for opening the issue!

Stimulus calls your application’s code in a few different places:

• When a controller is created, Stimulus calls the controller’s constructor function and the Controller#initialize() method
• When an element with a data-controller annotation appears in the DOM, Stimulus calls Controller#connect(); when it disappears, Controller#disconnect()
• When an event is dispatched on an element with a matching data-action annotation, Stimulus calls the controller’s corresponding action method

In each case, these calls happen inside a try ... catch block. If they didn’t, an error in any controller constructor, lifecycle callback, or action method would halt JavaScript execution, leaving Stimulus in an indeterminate state and possibly preventing subsequent callbacks or action methods from being invoked.

Inside the catch block, these errors make their way to a single endpoint, the Application#handleError() method. By default, this method calls console.error() to display a nicely formatted message, in red, including the error message, backtrace, controller, identifier, event, and element:

So this may be a case of each of us having a different idea of what it means for a framework to be “developer friendly.” My opinion is that it is more friendly for the framework not to break when application code raises an exception.

I also think our error logging is better, and that it’s a mischaracterization to say errors are “suppressed” when they appear by default in the browser’s console.

I admit that documentation around the handleError hook may be lacking. I’d love to discuss how we can improve it.

It’s also quite possible there’s a solution I haven’t considered that would let us have the best of both worlds: framework resilience against application errors and automatic compatibility with tools like Sentry. Again, I’d love to discuss this, if you have any ideas.

Until then, closing this issue since things are working as designed.

[ghiculescu]

@sstephenson I just stumbled across this after being stung by something similar in production.

In terms of docs, I have two ideas on how to make this more visible.

1. A new section in https://stimulus.hotwire.dev/handbook/installing that contains @javan 's code snippet from Add Application.prototype.handleError #53
2. A new reference page https://stimulus.hotwire.dev/reference

I tend to think option 1 is worth doing; option 2 feels a bit much.

It’s also quite possible there’s a solution I haven’t considered that would let us have the best of both worlds: framework resilience against application errors and automatic compatibility with tools like Sentry.

Most tools I've seen set window.onerror. So you could change https://github.com/hotwired/stimulus/blob/master/packages/@stimulus/core/src/application.ts#L72 to something like this:

  handleError(error: Error, message: string, detail: object) {
    this.logger.error(`%s\n\n%o\n\n%o`, message, error, detail)

    window.onerror && window.onerror(message, null, null, null, error)
  }

Happy to put together PRs for either/both of these changes, if you'd be open to them?