Update argument handling

This commit updates the argument handling:

* To match [RFC PR #620](emberjs/rfcs#620)
  * Returns `null` on `null` or `undefined`
  * Use development mode assertions for other invalid arguments
  * Add test for modifiers usage
  * Add test for `...attributes` usage
* To match more closely the likely Ember side implementation
  * Handle argument errors at runtime
  * `owner.hasRegistration('helper:element') === true`

Since the argument errors are moved to runtime, this also removes
the need for separate node tests. Previously, that also caused an
error when `ember-auto-import` is included. Since we removed the
`qunit` dependency we can now include `ember-auto-import` to align
with the default addon blueprint.

See also https://discordapp.com/channels/480462759797063690/485447409296736276/704799818316251216

Closes #6

.github/workflows/build.yml

@@ -51,9 +51,7 @@ jobs:
         run: yarn lint:js
       - name: Build
         run: yarn ember build --environment test
-      - name: Run test (node)
-        run: yarn test:node
-      - name: Run test (ember)
+      - name: Run test
         # Due to a bug in ember-cli, running `ember test` with `--path` doesn't set `EMBER_ENV=test`
         # See https://github.com/ember-cli/ember-cli/issues/8922
-        run: EMBER_ENV=test yarn test:ember --path dist
+        run: EMBER_ENV=test yarn test --path dist

CONTRIBUTING.md

@@ -14,10 +14,9 @@
 
 ## Running tests
 
-* `yarn test:ember` – Runs the test suite on the current Ember version
-* `yarn test:ember --server` – Runs the test suite in "watch mode"
+* `yarn test` – Runs the test suite on the current Ember version
+* `yarn test --server` – Runs the test suite in "watch mode"
 * `yarn test:all` – Runs the test suite against multiple Ember versions
-* `yarn test:node` – Runs the additional test suite for syntax errors
 
 ## Running the dummy application
 

README.md

@@ -5,17 +5,54 @@ ember-element-helper
 
 Dynamic element helper for Glimmer templates.
 
-See [this RFC comment](https://github.com/emberjs/rfcs/pull/389#issuecomment-429691544)
-for more context and motivation.
+This addon provides a ~~polyfill~~ high fidelity reference implementation of
+[RFC #389](https://github.com/emberjs/rfcs/pull/389), including the proposed
+amendments in [RFC PR #620](https://github.com/emberjs/rfcs/pull/620).
 
+Please note that the while [RFC #389](https://github.com/emberjs/rfcs/pull/389)
+has been approved, it has not been implmented in Ember.js yet. As such, the
+feature is still subject to change based on implementation feedback. In
+addition, the amendments in [RFC PR #620](https://github.com/emberjs/rfcs/pull/620)
+are still pending approval at this point.
+
+When this feature is implemented in Ember.js, we will release a 1.0 version of
+this addon as a true polyfill for the feature, allowing the feature to be used
+on older Ember.js versions and be completely inert on newer versions where the
+official implementation is available.
 
 Compatibility
 ------------------------------------------------------------------------------
 
-* Ember.js v3.4 or above
+* Ember.js v3.4 or above (with some limitations, see below)
 * Ember CLI v2.13 or above
 * Node.js v10 or above
 
+Limitations
+------------------------------------------------------------------------------
+
+This implementation has the following known limitations:
+
+* By default, an auto-generated `id` attribute will be added to the element
+  (e.g. `id="ember123"`). It is possible to override this by providing an
+  `id` attribute when invoking the component (e.g. `<Tag id="my-id" />`).
+  However, it is not possible to remove the `id` attribute completely. The
+  proposed helper will not have this behavior, as such this should not be
+  relied upon (e.g. in CSS and `qunit-dom` selectors).
+
+* The element will have an `ember-view` class (i.e. `class="ember-view"`).
+  This is in addition and merged with the class attribute provided when
+  invoking the component (e.g. `<Tag class="my-class" />` will result in
+  something like `<div class="ember-view my-class" />`). It is not possible
+  to remove the `ember-view` class. The proposed helper will not have this
+  behavior, as such this should not be relied upon (e.g. in CSS and `qunit-dom`
+  selectors).
+
+* In Ember versions before 3.11, modifiers cannot be passed to the element,
+  even with addons such as the [modifier manager](https://github.com/ember-polyfills/ember-modifier-manager-polyfill)
+  and [on modifier](https://github.com/buschtoens/ember-on-modifier) polyfills
+  are used. Doing so requires [RFC #435](https://github.com/emberjs/rfcs/blob/master/text/0435-modifier-splattributes.md)
+  which is first available on Ember 3.11. This is an Ember.js limitation,
+  unrelated to this addon.
 
 Installation
 ------------------------------------------------------------------------------

addon/helpers/-element.js

@@ -1,36 +1,48 @@
 import Helper from '@ember/component/helper';
+import { assert, runInDebug } from '@ember/debug';
 
 function UNINITIALIZED() {}
 
 export default Helper.extend({
   init() {
     this._super(...arguments);
     this.tagName = UNINITIALIZED;
-    this.componentName = UNINITIALIZED;
+    this.componentName = null;
   },
 
-  compute([tagName]) {
-    if (tagName === this.tagName) {
-      return this.componentName;
-    } else if (typeof tagName !== 'string') {
-      let message = 'the argument passed to the `element` helper must be a string';
+  compute(params, hash) {
+    assert('The `element` helper takes a single positional argument', params.length === 1);
+    assert('The `element` helper does not take any named arguments', Object.keys(hash).length === 0);
 
-      try {
-        message += ` (you passed \`${tagName}\`)`;
-      } catch (e) {
-        // ignore
-      }
+    let tagName = params[0];
 
-      throw new Error(message);
-    } else {
+    if (tagName !== this.tagName) {
       this.tagName = tagName;
 
-      // return a different component name to force a teardown
-      if (this.componentName === '-dynamic-element') {
-        return this.componentName = '-dynamic-element-alt';
+      if (typeof tagName === 'string') {
+        // return a different component name to force a teardown
+        if (this.componentName === '-dynamic-element') {
+          this.componentName = '-dynamic-element-alt';
+        } else {
+          this.componentName = '-dynamic-element';
+        }
       } else {
-        return this.componentName = '-dynamic-element';
+        this.componentName = null;
+
+        runInDebug(() => {
+          let message = 'The argument passed to the `element` helper must be a string';
+
+          try {
+            message += ` (you passed \`${tagName}\`)`;
+          } catch (e) {
+            // ignore
+          }
+
+          assert(message, tagName === undefined || tagName === null);
+        });
       }
     }
+
+    return this.componentName;
   }
 });

addon/helpers/element.js

@@ -0,0 +1,17 @@
+import { helper } from '@ember/component/helper';
+import { assert } from '@ember/debug';
+
+export default helper(function() {
+  // This helper (`element`, as opposed to `-element`) mostly exists to satisfy
+  // things like `owner.hasRegistration('helper:element')`. The AST transform
+  // replaces all usages of `(element ...)` into `(component (-element ...))`
+  // so if this helper is invoked directly, something is wrong.
+
+  assert(
+    'The `element` helper polyfill encounted an unexpected error. ' +
+    'Please report the issue at http://github.com/tildeio/ember-element-helper ' +
+    'with the usage and conditions that caused this error.'
+  );
+
+  return null;
+});

app/helpers/element.js

@@ -0,0 +1 @@
+export { default } from 'ember-element-helper/helpers/element';

ember-cli-build.js

@@ -14,5 +14,9 @@ module.exports = function(defaults) {
     behave. You most likely want to be modifying `./index.js` or app's build file
   */
 
+  if (app.env === 'test') {
+    app.import('node_modules/ember-source/dist/ember-template-compiler.js', { type: 'test' });
+  }
+
   return app.toTree();
 };

lib/element-helper-syntax-plugin.js

@@ -6,11 +6,6 @@ module.exports = class ElementHelperSyntaxPlugin {
     this.syntax.traverse(ast, {
       BlockStatement: {
         enter(node) {
-          // check this before we push the new locals
-          if (isElementHelper(node, locals)) {
-            throw error('does not take a block', node);
-          }
-
           locals.push(node.program.blockParams);
         },
 
@@ -49,8 +44,6 @@ module.exports = class ElementHelperSyntaxPlugin {
 }
 
 function transformParts(node, b) {
-  validateElementHelperArgs(node);
-
   return {
     // path
     path: b.path('component', node.path.loc),
@@ -66,9 +59,11 @@ function transformParts(node, b) {
       node.loc
     )],
     // hash
-    hash: b.hash([
-      b.pair('tagName', node.params[0], node.params[0].loc)
-    ], node.params[0].loc)
+    hash: node.params.length > 0 ?
+      b.hash([
+        b.pair('tagName', node.params[0], node.params[0].loc)
+      ], node.params[0].loc) :
+      b.hash(),
   };
 }
 
@@ -81,29 +76,3 @@ function isElementHelper(node, locals) {
     node.path.original === 'element' &&
     !hasLocalVariable('element', locals);
 }
-
-function validateElementHelperArgs(node) {
-  if (node.params.length !== 1) {
-    throw error('requires exactly one positional argument', node);
-  }
-
-  if (node.hash.pairs.length !== 0) {
-    throw error('does not accept any named arguments', node);
-  }
-}
-
-function error(reason, node) {
-  let message = `the \`element\` helper ${reason}`;
-
-  if (node.loc && node.loc.source !== '(synthetic)' && node.loc.start) {
-    let { line, column } = node.loc.start;
-
-    if (line !== undefined && column !== undefined) {
-      message += ` (on line ${line} column ${column})`;
-    } else if (line !== undefined) {
-      message += ` (on line ${line})`;
-    }
-  }
-
-  return new Error(message);
-}

package.json

@@ -17,21 +17,21 @@
     "lint:hbs": "ember-template-lint .",
     "lint:js": "eslint .",
     "start": "ember serve",
-    "test": "yarn test:node && yarn test:ember",
-    "test:node": "qunit node-tests",
-    "test:ember": "ember test",
+    "test": "ember test",
     "test:all": "ember try:each"
   },
   "dependencies": {
     "ember-cli-babel": "^7.17.2",
-    "ember-cli-htmlbars": "^4.2.2"
+    "ember-cli-htmlbars": "^4.2.2",
+    "ember-compatibility-helpers": "^1.2.1"
   },
   "devDependencies": {
     "@ember/optional-features": "^1.3.0",
     "@glimmer/component": "^1.0.0",
     "@glimmer/tracking": "^1.0.0",
     "babel-eslint": "^10.0.3",
     "broccoli-asset-rev": "^3.0.0",
+    "ember-auto-import": "^1.5.3",
     "ember-cli": "~3.16.1",
     "ember-cli-dependency-checker": "^3.2.0",
     "ember-cli-eslint": "^5.1.0",
@@ -51,7 +51,6 @@
     "eslint-plugin-ember": "^7.7.2",
     "eslint-plugin-node": "^11.0.0",
     "loader.js": "^4.7.0",
-    "qunit": "*",
     "qunit-dom": "^1.0.0"
   },
   "engines": {

tests/dummy/app/templates/components/element-receiver.hbs

@@ -1 +1 @@
-<@tag id="content">{{yield}}</@tag>
+<@tag id="content" ...attributes>{{yield}}</@tag>