Add 'on' keyword support and related tests

Author: NullVoxPopuli

package.json

@@ -95,7 +95,7 @@
     "@babel/preset-env": "^7.16.11",
     "@babel/types": "^7.22.5",
     "@embroider/macros": "^1.19.6",
-    "@embroider/shared-internals": "^2.5.0",
+    "@embroider/shared-internals": "^2.9.2",
     "@embroider/vite": "^1.5.0",
     "@eslint/js": "^9.21.0",
     "@glimmer/component": "workspace:*",

packages/@ember/template-compiler/lib/compile-options.ts

@@ -1,10 +1,12 @@
+import { on } from '@ember/modifier';
+import type { AST } from '@glimmer/syntax';
 import { assert } from '@ember/debug';
 import {
   RESOLUTION_MODE_TRANSFORMS,
   STRICT_MODE_KEYWORDS,
   STRICT_MODE_TRANSFORMS,
 } from './plugins/index';
-import type { EmberPrecompileOptions, PluginFunc } from './types';
+import type { EmberPrecompileOptions, JSUtils, PluginFunc } from './types';
 import COMPONENT_NAME_SIMPLE_DASHERIZE_CACHE from './dasherize-component-name';
 
 let USER_PLUGINS: PluginFunc[] = [];
@@ -13,11 +15,19 @@ function malformedComponentLookup(string: string) {
   return string.indexOf('::') === -1 && string.indexOf(':') > -1;
 }
 
+export const keywords = {
+  on,
+};
+
+type Options = EmberPrecompileOptions &
+  Partial<EmberPrecompileOptions> & {
+    meta?: EmberPrecompileOptions['meta'] & { jsutils?: JSUtils };
+  };
+
 function buildCompileOptions(_options: EmberPrecompileOptions): EmberPrecompileOptions {
   let moduleName = _options.moduleName;
 
-  let options: EmberPrecompileOptions & Partial<EmberPrecompileOptions> = {
-    meta: {},
+  let options: Options = {
     isProduction: false,
     plugins: { ast: [] },
     ..._options,
@@ -34,6 +44,34 @@ function buildCompileOptions(_options: EmberPrecompileOptions): EmberPrecompileO
     },
   };
 
+  options.meta ||= {};
+  options.meta.jsutils ||= {
+    /**
+     * NOTE: when stepping through lexicalScope, or other callbacks here,
+     *       we first detect the keywords as "not in scope",
+     *       and that is what we want, so that we can import them.
+     */
+    bindImport(
+      module: string,
+      name: string,
+      node: AST.ElementModifierStatement | AST.MustacheStatement | AST.SubExpression
+    ) {
+      if (module === '@ember/modifier' && name === 'on') {
+        /**
+         * We rely on an old JS technique of assiging properties to functions.
+         * Since template() is what was used to runtime-compile the template,
+         * we know it to be in scope.
+         */
+        if (node.path.type === 'PathExpression') {
+          node.path.original = 'template.on';
+          return;
+        }
+      }
+
+      throw new Error(`Unknown import ${name} from module ${module}`);
+    },
+  };
+
   if ('eval' in options) {
     const localScopeEvaluator = options.eval as (value: string) => unknown;
     const globalScopeEvaluator = (value: string) => new Function(`return ${value};`)();
@@ -52,7 +90,7 @@ function buildCompileOptions(_options: EmberPrecompileOptions): EmberPrecompileO
   if ('scope' in options) {
     const scope = (options.scope as () => Record<string, unknown>)();
 
-    options.lexicalScope = (variable: string) => variable in scope;
+    options.lexicalScope = (variable: string) => variable in scope || variable in keywords;
 
     delete options.scope;
   }

packages/@ember/template-compiler/lib/plugins/auto-import-builtins.ts

@@ -0,0 +1,39 @@
+import type { AST, ASTPlugin } from '@glimmer/syntax';
+import type { EmberASTPluginEnvironment } from '../types';
+import { isPath, trackLocals } from './utils';
+
+/**
+ @module ember
+*/
+
+/**
+  A Glimmer2 AST transformation that makes importable keywords work
+
+  @private
+  @class TransformActionSyntax
+*/
+
+export default function autoImportBuiltins(env: EmberASTPluginEnvironment): ASTPlugin {
+  let { hasLocal, visitor } = trackLocals(env);
+
+  return {
+    name: 'auto-import-built-ins',
+
+    visitor: {
+      ...visitor,
+      ElementModifierStatement(node: AST.ElementModifierStatement) {
+        if (isOn(node, hasLocal)) {
+          // @ts-expect-error may not exist in all environments (the deprecated runtime compiler, for example)
+          env.meta?.jsutils?.bindImport?.('@ember/modifier', 'on', node, { name: 'on' });
+        }
+      },
+    },
+  };
+}
+
+function isOn(
+  node: AST.ElementModifierStatement | AST.MustacheStatement | AST.SubExpression,
+  hasLocal: (k: string) => boolean
+) {
+  return isPath(node.path) && node.path.original === 'on' && !hasLocal('on');
+}

packages/@ember/template-compiler/lib/plugins/index.ts

@@ -9,8 +9,10 @@ import TransformInElement from './transform-in-element';
 import TransformQuotedBindingsIntoJustBindings from './transform-quoted-bindings-into-just-bindings';
 import TransformResolutions from './transform-resolutions';
 import TransformWrapMountAndOutlet from './transform-wrap-mount-and-outlet';
+import AutoImportBuiltins from './auto-import-builtins';
 
 export const INTERNAL_PLUGINS = {
+  AutoImportBuiltins,
   AssertAgainstAttrs,
   AssertAgainstNamedOutlets,
   AssertInputHelperWithoutBlock,
@@ -40,6 +42,7 @@ export const RESOLUTION_MODE_TRANSFORMS = Object.freeze([
 ]);
 
 export const STRICT_MODE_TRANSFORMS = Object.freeze([
+  AutoImportBuiltins,
   TransformQuotedBindingsIntoJustBindings,
   AssertReservedNamedArguments,
   TransformActionSyntax,

packages/@ember/template-compiler/lib/template.ts

@@ -3,7 +3,7 @@ import { precompile as glimmerPrecompile } from '@glimmer/compiler';
 import type { SerializedTemplateWithLazyBlock } from '@glimmer/interfaces';
 import { setComponentTemplate } from '@glimmer/manager';
 import { templateFactory } from '@glimmer/opcode-compiler';
-import compileOptions from './compile-options';
+import compileOptions, { keywords } from './compile-options';
 import type { EmberPrecompileOptions } from './types';
 
 type ComponentClass = abstract new (...args: any[]) => object;
@@ -237,8 +237,8 @@ export function template(
   providedOptions?: BaseTemplateOptions | BaseClassTemplateOptions<any>
 ): object {
   const options: EmberPrecompileOptions = { strictMode: true, ...providedOptions };
-  const evaluate = buildEvaluator(options);
 
+  const evaluate = buildEvaluator(options);
   const normalizedOptions = compileOptions(options);
   const component = normalizedOptions.component ?? templateOnly();
 
@@ -250,24 +250,32 @@ export function template(
   return component;
 }
 
+Object.assign(template, keywords);
+
 const evaluator = (source: string) => {
-  return new Function(`return  ${source}`)();
+  return new Function('template', `return  ${source}`)(template);
 };
 
-function buildEvaluator(options: Partial<EmberPrecompileOptions> | undefined) {
-  if (options === undefined) {
-    return evaluator;
-  }
-
+/**
+ * @param options
+ * @returns
+ */
+function buildEvaluator(options: Partial<EmberPrecompileOptions>) {
   if (options.eval) {
     return options.eval;
   } else {
-    const scope = options.scope?.();
+    /**
+     * This is ran before the template is compiled,
+     * so we cannot use any information gathered during template compilation.
+     */
+    let scope = options.scope?.();
 
     if (!scope) {
       return evaluator;
     }
 
+    scope = Object.assign({}, keywords, scope);
+
     return (source: string) => {
       const argNames = Object.keys(scope);
       const argValues = Object.values(scope);

packages/@ember/template-compiler/lib/types.ts

@@ -1,4 +1,5 @@
 import type {
+  AST,
   ASTPluginEnvironment,
   builders,
   PrecompileOptions,
@@ -23,6 +24,10 @@ interface Plugins {
   ast: PluginFunc[];
 }
 
+export interface JSUtils {
+  bindImport(module: string, name: string, node: AST.Node): void;
+}
+
 export interface EmberPrecompileOptions extends PrecompileOptions {
   isProduction?: boolean;
   moduleName?: string;

packages/@ember/template-compiler/package.json

@@ -12,6 +12,8 @@
     "@ember/-internals": "workspace:*",
     "@ember/component": "workspace:*",
     "@ember/debug": "workspace:*",
+    "@ember/modifier": "workspace:*",
+    "@ember/helper": "workspace:*",
     "@glimmer/compiler": "workspace:*",
     "@glimmer/env": "workspace:*",
     "@glimmer/interfaces": "workspace:*",

packages/@glimmer-workspace/integration-tests/test/keywords/on-runtime-test.ts

@@ -0,0 +1,64 @@
+import { castToBrowser } from '@glimmer/debug-util';
+import { jitSuite, RenderTest, test } from '@glimmer-workspace/integration-tests';
+
+import { template } from '@ember/template-compiler/runtime';
+
+class KeywordOn extends RenderTest {
+  static suiteName = 'keyword modifier: on (runtime)';
+
+  @test
+  'explicit scope'(assert: Assert) {
+    let handleClick = () => {
+      assert.step('success');
+    };
+
+    const compiled = template('<button {{on "click" handleClick}}>Click</button>', {
+      strictMode: true,
+      scope: () => ({
+        handleClick,
+      }),
+    });
+
+    this.renderComponent(compiled);
+
+    castToBrowser(this.element, 'div').querySelector('button')!.click();
+    assert.verifySteps(['success']);
+  }
+
+  @test
+  'implicit scope'(assert: Assert) {
+    let handleClick = () => {
+      assert.step('success');
+    };
+
+    hide(handleClick);
+
+    const compiled = template('<button {{on "click" handleClick}}>Click</button>', {
+      strictMode: true,
+      eval() {
+        return eval(arguments[0]);
+      },
+    });
+
+    this.renderComponent(compiled);
+
+    castToBrowser(this.element, 'div').querySelector('button')!.click();
+    assert.verifySteps(['success']);
+  }
+}
+
+jitSuite(KeywordOn);
+
+/**
+ * This function is used to hide a variable from the transpiler, so that it
+ * doesn't get removed as "unused". It does not actually do anything with the
+ * variable, it just makes it be part of an expression that the transpiler
+ * won't remove.
+ *
+ * It's a bit of a hack, but it's necessary for testing.
+ *
+ * @param variable The variable to hide.
+ */
+const hide = (variable: unknown) => {
+  new Function(`return (${JSON.stringify(variable)});`);
+};

packages/@glimmer-workspace/integration-tests/test/keywords/on-test.ts

@@ -0,0 +1,99 @@
+import { castToBrowser } from '@glimmer/debug-util';
+import { jitSuite, RenderTest, test } from '@glimmer-workspace/integration-tests';
+import { setModifierManager, modifierCapabilities } from '@glimmer/manager';
+
+import { template } from '@ember/template-compiler/runtime';
+import { on } from '@ember/modifier';
+
+class KeywordOn extends RenderTest {
+  static suiteName = 'keyword modifier: on';
+
+  /**
+   * We require the babel compiler to emit keywords, so this is actually no different than normal usage
+   * prior to RFC 997.
+   *
+   * We are required to have the compiler that emits this low-level format to detect if on is in scope and then
+   * _not_ add the `on` modifier from `@ember/modifier` import.
+   */
+  @test
+  'it works'(assert: Assert) {
+    let handleClick = () => {
+      assert.step('success');
+    };
+
+    const compiled = template('<button {{on "click" handleClick}}>Click</button>', {
+      strictMode: true,
+      scope: () => ({
+        handleClick,
+        on,
+      }),
+    });
+
+    this.renderComponent(compiled);
+
+    castToBrowser(this.element, 'div').querySelector('button')!.click();
+    assert.verifySteps(['success']);
+  }
+
+  @test
+  'it works with the runtime compiler'(assert: Assert) {
+    let handleClick = () => {
+      assert.step('success');
+    };
+
+    hide(handleClick);
+
+    const compiled = template('<button {{on "click" handleClick}}>Click</button>', {
+      strictMode: true,
+      eval() {
+        return eval(arguments[0]);
+      },
+    });
+
+    this.renderComponent(compiled);
+
+    castToBrowser(this.element, 'div').querySelector('button')!.click();
+    assert.verifySteps(['success']);
+  }
+
+  @test
+  'can be shadowed'(assert: Assert) {
+    let on = setModifierManager(() => {
+      return {
+        capabilities: modifierCapabilities('3.22'),
+        createModifier() {
+          assert.step('shadowed:success');
+        },
+        installModifier() {},
+        updateModifier() {},
+        destroyModifier() {},
+      };
+    }, {});
+
+    const compiled = template('<button {{on "click"}}>Click</button>', {
+      strictMode: true,
+      scope: () => ({ on }),
+    });
+
+    this.renderComponent(compiled);
+
+    castToBrowser(this.element, 'div').querySelector('button')!.click();
+    assert.verifySteps(['shadowed:success']);
+  }
+}
+
+jitSuite(KeywordOn);
+
+/**
+ * This function is used to hide a variable from the transpiler, so that it
+ * doesn't get removed as "unused". It does not actually do anything with the
+ * variable, it just makes it be part of an expression that the transpiler
+ * won't remove.
+ *
+ * It's a bit of a hack, but it's necessary for testing.
+ *
+ * @param variable The variable to hide.
+ */
+const hide = (variable: unknown) => {
+  new Function(`return (${JSON.stringify(variable)});`);
+};