Merge pull request #21091 from crazylogic03/fix/on-modifier-error-message

[BUGFIX beta] Fix 'on' modifier error message regression

Author: NullVoxPopuli

package.json

@@ -390,4 +390,4 @@
     }
   },
   "packageManager": "pnpm@10.5.0"
-}
+}

packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js

@@ -2,6 +2,8 @@ import { moduleFor, RenderingTestCase, runTask } from 'internal-test-helpers';
 import { getInternalModifierManager } from '@glimmer/manager';
 import { on } from '@glimmer/runtime';
 
+import { DEBUG } from '@glimmer/env';
+
 import { Component } from '../../utils/helpers';
 
 moduleFor(
@@ -31,7 +33,7 @@ moduleFor(
       );
     }
 
-    ['@test it adds an event listener'](assert) {
+    [`@test it adds an event listener`](assert) {
       let count = 0;
 
       this.render('<button {{on "click" this.callback}}>Click Me</button>', {
@@ -242,6 +244,30 @@ moduleFor(
 
       this.assertCounts({ adds: 1, removes: 1 });
     }
+
+    [`@test it throws a helpful error when callback is undefined`](assert) {
+      if (DEBUG) {
+        let expectedMessage =
+          /You must pass a function as the second argument to the `on` modifier/;
+        assert.throws(() => {
+          this.render('<button {{on "click" undefined}}>Click Me</button>');
+        }, expectedMessage);
+      } else {
+        assert.expect(0);
+      }
+    }
+
+    [`@test it throws a helpful error when callback is null`](assert) {
+      if (DEBUG) {
+        let expectedMessage =
+          /You must pass a function as the second argument to the `on` modifier/;
+        assert.throws(() => {
+          this.render('<button {{on "click" null}}>Click Me</button>');
+        }, expectedMessage);
+      } else {
+        assert.expect(0);
+      }
+    }
   }
 );
 

packages/@glimmer/runtime/lib/modifiers/on.ts

@@ -68,17 +68,24 @@ export class OnModifierState {
       () => 'You must pass a valid DOM event name as the first argument to the `on` modifier'
     );
 
+    let arg1 = args.positional[1];
+    let userProvidedCallback = check(
+      arg1 ? valueForRef(arg1) : undefined,
+      CheckFunction,
+      (actual) => {
+        return `You must pass a function as the second argument to the \`on\` modifier; you passed ${
+          actual === null ? 'null' : typeof actual
+        }. While rendering:\n\n${args.positional[1]?.debugLabel ?? `{unlabeled value}`}`;
+      }
+    ) as EventListener;
+
     localAssert(
-      args.positional[1],
-      'You must pass a function as the second argument to the `on` modifier'
+      typeof userProvidedCallback === 'function',
+      `You must pass a function as the second argument to the \`on\` modifier; you passed ${
+        userProvidedCallback === null ? 'null' : typeof userProvidedCallback
+      }. While rendering:\n\n${args.positional[1]?.debugLabel ?? `{unlabeled value}`}`
     );
 
-    let userProvidedCallback = check(valueForRef(args.positional[1]), CheckFunction, (actual) => {
-      return `You must pass a function as the second argument to the \`on\` modifier; you passed ${
-        actual === null ? 'null' : typeof actual
-      }. While rendering:\n\n${args.positional[1]?.debugLabel ?? `{unlabeled value}`}`;
-    }) as EventListener;
-
     if (DEBUG && args.positional.length !== 2) {
       throw new Error(
         `You can only pass two positional arguments (event name and callback) to the \`on\` modifier, but you provided ${args.positional.length}. Consider using the \`fn\` helper to provide additional arguments to the \`on\` callback.`