Errors are not caught/handled within value changed & target connected callbacks #711

lb- · 2023-08-03T22:18:46Z

I have recently set up a controller where we want to ensure that a specific target exists and found what could be a bug in how the error handling works in Stimulus.

According to the documentation on error handling;

All calls from Stimulus to your application’s code are wrapped in a try ... catch block.
If your code throws an error, it will be caught by Stimulus and logged to the browser console, including extra detail such as the controller name and event or lifecycle function being called. If you use an error tracking system that defines window.onerror, Stimulus will also pass the error on to it.

Problem

Errors in the <some>valueChanged callbacks are not being caught like all other errors and result in code that is hard to debug, log and test.

Steps to reproduce (basic)

Set up a simple controller with a value and throw any error inside the value changed callback.

// reveal-controller.js
import { Controller } from '@hotwired/stimulus';

export default class extends Controller {
  static values = { closed: Boolean };

  closedValueChanged() {
    throw new Error('Is this error caught?');
  }
}

Add the HTML to connect to the controller & load in a browser.

<div class="container" data-controller="reveal"></div>

Expected: Error should be handled and captured similar to errors in other methods. Actual: Error is not caught.

// console result
stimulus.js:2023 application #starting
stimulus.js:2023 reveal #initialize
reveal-controller.js:7 Uncaught (in promise) Error: Is this error caught?
    at t.value (reveal-controller.js:7:11)
    at M.invokeChangedCallback (stimulus.js:1115:31)
    at M.invokeChangedCallbacksForDefaultValues (stimulus.js:1100:22)
    at M.start (stimulus.js:1057:14)
    at B.connect (stimulus.js:1376:28)
    at D.connectContextForScope (stimulus.js:1550:17)
    at H.scopeConnected (stimulus.js:1924:20)
    at j.elementMatchedValue (stimulus.js:1840:27)
    at A.tokenMatched (stimulus.js:946:27)
    at S.tokenMatched (stimulus.js:877:23)
value @ reveal-controller.js:7
invokeChangedCallback @ stimulus.js:1115
invokeChangedCallbacksForDefaultValues @ stimulus.js:1100
start @ stimulus.js:1057
connect @ stimulus.js:1376

As a comparison, using this HTML and clicking the button will result in a caught error.

// reveal-controller.js
import { Controller } from '@hotwired/stimulus';

export default class extends Controller {
  static values = { closed: Boolean };

  close() {
    throw new Error('Is this error caught?');
  }
}

<div class="container" data-controller="reveal">
  <button data-action="click->reveal#close" type="button">Close</button>
</div>

// Observe the handleError catches this error
stimulus.js:2023 application #starting
stimulus.js:2023 reveal #initialize
stimulus.js:2023 reveal #connect
stimulus.js:2023 application #start
stimulus.js:2018 Error invoking action "click->reveal#close"

Error: Is this error caught?
    at t.value (reveal-controller.js:7:11)
    at f.invokeWithEvent (stimulus.js:363:25)
    at f.handleEvent (stimulus.js:329:18)
    at a.handleEvent (stimulus.js:31:25)

{identifier: 'reveal', controller: t, element: div.container, index: 0, event: PointerEvent}
handleError @ stimulus.js:2018
handleError @ stimulus.js:1424
invokeWithEvent @ stimulus.js:369

Steps to reproduce (full details)

Consider this controller code.

// reveal-controller.js
import { Controller } from '@hotwired/stimulus';

export default class extends Controller {
  static values = { closed: Boolean };
  static targets = ['content', 'toggle'];

  connect() {
    this.closedValue ? this.close() : this.open();
  }

  close() {
    this.toggleTarget.setAttribute('aria-expanded', 'false');
    this.contentTarget.hidden = true;
    this.closedValue = true;
  }

  open() {
    this.toggleTarget.setAttribute('aria-expanded', 'true');
    this.contentTarget.hidden = false;
    this.closedValue = false;
  }

  toggle() {
    this.closedValue ? this.open() : this.close();
  }
}

And this HTML.

  <body>
    <section class="section">
      <div class="container" data-controller="reveal">
        <h2>Information</h2>
        <button data-action="click->reveal#toggle" type="button">Toggle</button>
        <p class="content" data-target="content">Additional content...</p>
      </div>
    </section>
  </body>

Observe that there is no toggle target set in the HTML, accessing this in the controller will cause a caught error (as expected).

stimulus.js:2023 application #starting
stimulus.js:2023 reveal #initialize
stimulus.js:2018 Error connecting controller

Error: Missing target element "toggle" for "reveal" controller
    at t.get (stimulus.js:2154:27)
    at t.value (reveal-controller.js:18:10)
    at t.value (reveal-controller.js:8:44)
    at B.connect (stimulus.js:1380:29)
    at D.connectContextForScope (stimulus.js:1550:17)
    at H.scopeConnected (stimulus.js:1924:20)
    at j.elementMatchedValue (stimulus.js:1840:27)
    at A.tokenMatched (stimulus.js:946:27)
    at S.tokenMatched (stimulus.js:877:23)
    at stimulus.js:871:40

{identifier: 'reveal', controller: t, element: div.container}
handleError @ stimulus.js:2018
handleError @ stimulus.js:1424
connect @ stimulus.js:1384
connectContextForScope @ stimulus.js:1550
...

Now, refactor this same controller to use value changed callbacks.

import { Controller } from '@hotwired/stimulus';

export default class extends Controller {
  static values = { closed: Boolean };
  static targets = ['content', 'toggle'];

  /**
   * Will cause uncaught error if a target does not exist.
   */
  closedValueChanged(shouldClose) {
    if (shouldClose) {
      this.toggleTarget.setAttribute('aria-expanded', 'false');
      this.contentTarget.hidden = true;
    } else {
      this.toggleTarget.setAttribute('aria-expanded', 'true');
      this.contentTarget.hidden = false;
    }
  }

  close() {
    this.closedValue = true;
  }

  open() {
    this.closedValue = false;
  }

  toggle() {
    this.closedValue = !this.closedValue;
  }
}

With the same HTML, we now get an uncaught error, this does not get handled by Stimulus, not logged as expected and also makes it very difficult to unit test this scenario. The message is there but there is no way for this to be caught by any code easily.

stimulus.js:2023 application #starting
stimulus.js:2023 reveal #initialize
stimulus.js:2154 Uncaught (in promise) Error: Missing target element "toggle" for "reveal" controller
    at t.get (stimulus.js:2154:27)
    at t.value (reveal-controller.js:15:12)
    at M.invokeChangedCallback (stimulus.js:1115:31)
    at M.invokeChangedCallbacksForDefaultValues (stimulus.js:1100:22)
    at M.start (stimulus.js:1057:14)
    at B.connect (stimulus.js:1376:28)
    at D.connectContextForScope (stimulus.js:1550:17)
    at H.scopeConnected (stimulus.js:1924:20)
    at j.elementMatchedValue (stimulus.js:1840:27)
    at A.tokenMatched (stimulus.js:946:27)
get @ stimulus.js:2154
value @ reveal-controller.js:15
invokeChangedCallback @ stimulus.js:1115
invokeChangedCallbacksForDefaultValues @ stimulus.js:1100
start @ stimulus.js:1057
connect @ stimulus.js:1376
connectContextForScope @ stimulus.js:1550
scopeConnected @ stimulus.js:1924

I can kind of work around this by doing a check of targets that we care about in the initalize callback (called before the first changed callback).


  initialize() {
    // attempt to 'fail early' if the controller is misconfigured
    this.toggleTarget;
  }

We still get the uncaught error though, but at least we get a caught one just before it. However, this is not going to help when unit testing this case as the process will still fail.

Potential solution

I think the issue is that the private method invokeChangedCallback does not pass the error to handleError. Instead it just throws it, but I cannot see any places where this method is called that catch this error, causing it to bubble up.

stimulus/src/core/value_observer.ts

Lines 98 to 103 in 7b810ec

    
           } catch (error) { 
        
             if (error instanceof TypeError) { 
        
               error.message = `Stimulus Value "${this.context.identifier}.${descriptor.name}" - ${error.message}` 
        
             } 
        
             throw error

Maybe we can avoid this by passing the error to the handleError instead as this pattern is used by other code.

-      } catch (error) {
+      } catch (error: any) {
        if (error instanceof TypeError) {
          error.message = `Stimulus Value "${this.context.identifier}.${descriptor.name}" - ${error.message}`
        }
-        throw error
+        this.context.handleError(error, `calling ${changedMethodName} with ${JSON.stringify([rawValue, rawOldValue])}`, {/* detail... */})

Note: I have not tested this but just a guess.

The text was updated successfully, but these errors were encountered:

lb- · 2024-02-11T10:21:13Z

This appears to be an issue for target connected / disconnected callbacks also.

lb- · 2024-10-29T13:01:08Z

Would the Stimulus team be up for a PR for this, it's causing many issues for our Stimulus adoption in both testing and creating robust controllers.

lb- · 2024-10-29T13:05:05Z

For anyone writing unit tests, here's a way around it for now.

    app = Application.start(); // or however you want to instantiate your app in your code

    /**
     * Monkey patch the Context class's invokeControllerMethod method
     * so that we can catch errors thrown by the controller methods
     *
     * @see https://github.com/hotwired/stimulus/issues/711
     */
    const originalInvokeControllerMethod =
      Context.prototype.invokeControllerMethod;
    Context.prototype.invokeControllerMethod = function invokeControllerMethod(
      methodName,
      ...args
    ) {
      try {
        originalInvokeControllerMethod.call(this, methodName, ...args);
      } catch (error) {
        app.handleError({ error });
      }
    };

I use this in combination with a generic handleError callback that collects errors for testing.

let errors = []

// in your `setup` function or similar, or beforeAll /beforeEach
 app.handleError = (error, message, detail) => {
      errors.push({ error, message, detail });
    };

// in your afterEach

errors = [];

This allows us to test valid/expected thrown errors within *targetConnected callbacks. Note that this does not handle promises within the callbacks but it's good enough for now.

A similar workaround could probably be found for the value changed callbacks also.

lb- mentioned this issue Aug 17, 2023

Migrate breadcrumbs to Stimulus (RevealController / w-breadcrumb) wagtail/wagtail#10793

Merged

5 tasks

lb- mentioned this issue Feb 11, 2024

🎛️ Replace base ExpandingFormset functionality with Stimulus FormsetController w-formset inc. initial adoption wagtail/wagtail#11633

Closed

5 tasks

lb- changed the title ~~Errors are not caught/handled within value changed callbacks~~ Errors are not caught/handled within value changed & target connected callbacks Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errors are not caught/handled within value changed & target connected callbacks #711

Errors are not caught/handled within value changed & target connected callbacks #711

lb- commented Aug 3, 2023

lb- commented Feb 11, 2024

lb- commented Oct 29, 2024

lb- commented Oct 29, 2024

Errors are not caught/handled within value changed & target connected callbacks #711

Errors are not caught/handled within value changed & target connected callbacks #711

Comments

lb- commented Aug 3, 2023

Problem

Steps to reproduce (basic)

Steps to reproduce (full details)

Potential solution

lb- commented Feb 11, 2024

lb- commented Oct 29, 2024

lb- commented Oct 29, 2024