Repository: ember-codemods/ember-angle-brackets-codemod (js)

Issue:
@required vs required
The codemod is currently converting `required` arguments to `required` attributes, instead of `@required` arguments. This might be fine for `<Input>` (is it?), but certainly not for **all** components.

The same applies for `placeholder` vs. `@placeholder`.

The codemod also does something similar for `class`, but I suspect in that case it should be fine.

Pull request description:
fix migrating away from arguments to attributes for non-builtins
fixes #146 

Patch:
diff --git a/transforms/angle-brackets/transform.js b/transforms/angle-brackets/transform.js
index d741920..170a39a 100755
--- a/transforms/angle-brackets/transform.js
+++ b/transforms/angle-brackets/transform.js
@@ -12,7 +12,15 @@ const HTML_ATTRIBUTES = ['class', 'placeholder', 'required'];
 const BUILT_IN_COMPONENTS = ['link-to', 'input', 'textarea'];
 
 function isAttribute(key) {
-  return HTML_ATTRIBUTES.includes(key) || key.startsWith('data-');
+  return HTML_ATTRIBUTES.includes(key) || isDataAttribute(key);
+}
+
+function isDataAttribute(key) {
+  return key.startsWith('data-');
+}
+
+function isBuiltInComponent(key) {
+  return BUILT_IN_COMPONENTS.includes(key);
 }
 
 function isNestedComponentTagName(tagName) {
@@ -129,12 +137,12 @@ function transformToAngleBracket(env, fileInfo, config) {
   /**
    * Transform the attributes names & values properly
    */
-  function transformAttrs(attrs) {
+  function transformAttrs(tagName, attrs) {
     return attrs.map(a => {
       let _key = a.key;
       let _valueType = a.value.type;
       let _value;
-      if (!isAttribute(a.key)) {
+      if (!isAttribute(_key) || !isBuiltInComponent(tagName)) {
         _key = `@${_key}`;
       }
 
@@ -261,19 +269,13 @@ function transformToAngleBracket(env, fileInfo, config) {
     return attributes.concat(dataAttributes);
   }
 
-  function tranformValuelessDataParams(params) {
-    let dataAttributes = getDataAttributesFromParams(params);
-    let valuelessDataAttributes = dataAttributes.map(param =>
-      b.attr(param.parts[0], b.text(_EMPTY_STRING_))
-    );
-    return valuelessDataAttributes;
+  function hasValuelessDataParams(params) {
+    return getDataAttributesFromParams(params).length > 0;
   }
 
-  function transformNodeAttributes(node) {
-    let params = tranformValuelessDataParams(node.params);
-    let attributes = transformAttrs(node.hash.pairs);
-
-    return params.concat(attributes);
+  function transformNodeAttributes(tagName, node) {
+    let attributes = transformAttrs(tagName, node.hash.pairs);
+    return node.params.concat(attributes);
   }
 
   function getDataAttributesFromParams(params) {
@@ -305,10 +307,13 @@ function transformToAngleBracket(env, fileInfo, config) {
   }
 
   function transformNode(node) {
+    if (hasValuelessDataParams(node.params)) {
+      return;
+    }
     let selfClosing = node.type !== 'BlockStatement';
     const tagName = node.path.original;
 
-    if (config.skipBuiltInComponents && BUILT_IN_COMPONENTS.includes(tagName)) {
+    if (config.skipBuiltInComponents && isBuiltInComponent(tagName)) {
       return;
     }
 
@@ -331,7 +336,7 @@ function transformToAngleBracket(env, fileInfo, config) {
         attributes = transformLinkToAttrs(node.params);
       }
 
-      let namesParams = transformAttrs(node.hash.pairs);
+      let namesParams = transformAttrs(tagName, node.hash.pairs);
       attributes = attributes.concat(namesParams);
     } else {
       if (nodeHasPositionalParameters(node)) {
@@ -340,9 +345,8 @@ function transformToAngleBracket(env, fileInfo, config) {
         );
         return;
       }
-      attributes = transformNodeAttributes(node);
+      attributes = transformNodeAttributes(tagName, node);
     }
-
     return b.element(
       { name: newTagName, selfClosing },
       {


Test patch:
diff --git a/transforms/angle-brackets/test.js b/transforms/angle-brackets/test.js
index f153423..d92c4a3 100644
--- a/transforms/angle-brackets/test.js
+++ b/transforms/angle-brackets/test.js
@@ -109,29 +109,32 @@ test('data-attributes', () => {
 
   expect(runTest('data-attributes.hbs', input)).toMatchInlineSnapshot(`
     "
-        <XFoo data-foo={{true}} />
-        <XFoo data-test-selector={{true}} />
-        <XFoo data-test-selector={{post.id}} />
-        <XFoo @label=\\"hi\\" data-test-selector={{true}} />
-        <XFoo data-test-foo />
+        <XFoo @data-foo={{true}} />
+        <XFoo @data-test-selector={{true}} />
+        <XFoo @data-test-selector={{post.id}} />
+        <XFoo @label=\\"hi\\" @data-test-selector={{true}} />
+        {{x-foo data-test-foo }}
 
-        <XFoo data-foo={{true}}>
+        <XFoo @data-foo={{true}}>
           block
         </XFoo>
 
-        <XFoo data-test-selector={{true}}>
+        <XFoo @data-test-selector={{true}}>
           block
         </XFoo>
 
-        <XFoo data-test-selector={{post.id}}>
+        <XFoo @data-test-selector={{post.id}}>
           block
         </XFoo>
 
-        <Common::AccordionComponent data-test-accordion as |accordion|>
+        {{#common/accordion-component data-test-accordion as |accordion|}}
           block
-        </Common::AccordionComponent>
+        {{/common/accordion-component}}
 
-        <XFoo data-foo @name=\\"Sophie\\" />
+        {{x-foo
+          data-foo
+          name=\\"Sophie\\"
+        }}
       "
   `);
 });
@@ -174,13 +177,13 @@ test('deeply-nested-sub', () => {
    */
   expect(runTest('deeply-nested-sub.hbs', input)).toMatchInlineSnapshot(`
     "
-        <SomeComponent class={{concat foo (some-helper ted (some-dude bar (a b c)))}}>
+        <SomeComponent @class={{concat foo (some-helper ted (some-dude bar (a b c)))}}>
           help
         </SomeComponent>
-        <SomeComponent class={{concat foo (some-helper ted (some-dude bar (a b c)))}} />
-        <DeepComponent class={{concat foo (nice-helper ted (some-crazy bar (a d (d e f))))}} />
-        <SomeComponent class={{concat foo (some-helper bar)}} />
-        <SomeComponent class={{concat foo (some-helper bar quuz)}} />
+        <SomeComponent @class={{concat foo (some-helper ted (some-dude bar (a b c)))}} />
+        <DeepComponent @class={{concat foo (nice-helper ted (some-crazy bar (a d (d e f))))}} />
+        <SomeComponent @class={{concat foo (some-helper bar)}} />
+        <SomeComponent @class={{concat foo (some-helper bar quuz)}} />
         <SomeComponent @person={{hash name=\\"Sophie\\" age=1}} @message={{t \\"welcome\\" count=1}} />
         <SomeComponent @people={{array (hash name=\\"Alex\\" age=5 nested=(hash oldest=true amount=(format-currency 350 sign=\\"£\\")) disabled=(eq foo \\"bar\\")) (hash name=\\"Ben\\" age=4) (hash name=\\"Sophie\\" age=1)}} />
       "
@@ -216,7 +219,7 @@ test('entities', () => {
   expect(runTest('entities.hbs', input)).toMatchInlineSnapshot(`
     "
         &lt; &gt; &times;
-        <Foo data-a=\\"&quot;Foo&nbsp;&amp;&nbsp;Bar&quot;\\">&nbsp;Some text &gt;</Foo>
+        <Foo @data-a=\\"&quot;Foo&nbsp;&amp;&nbsp;Bar&quot;\\">&nbsp;Some text &gt;</Foo>
       "
   `);
 });
@@ -465,15 +468,15 @@ test('link-to-query-param', () => {
         <LinkTo @route=\\"posts\\" @query={{hash direction=\\"desc\\" showArchived=false}}>
           Recent Posts
         </LinkTo>
-        <LinkTo @route=\\"posts\\" data-test-foo>
+        {{#link-to data-test-foo \\"posts\\"}}
           Recent Posts
-        </LinkTo>
+        {{/link-to}}
         <LinkTo @route={{this.dynamicPath}} @query={{hash direction=\\"desc\\" showArchived=false}}>
           Recent Posts
         </LinkTo>
-        <LinkTo @route={{this.dynamicPath}} @query={{hash direction=\\"desc\\" showArchived=false}} data-test-foo>
+        {{#link-to data-test-foo this.dynamicPath (query-params direction=\\"desc\\" showArchived=false)}}
           Recent Posts
-        </LinkTo>
+        {{/link-to}}
         <LinkTo @query={{hash direction=\\"desc\\" showArchived=false}}>
           Recent Posts
         </LinkTo>
@@ -496,7 +499,7 @@ test('nested', () => {
 
   expect(runTest('nested.hbs', input)).toMatchInlineSnapshot(`
     "
-        <Ui::SiteHeader @user={{this.user}} class={{if this.user.isAdmin \\"admin\\"}} />
+        <Ui::SiteHeader @user={{this.user}} @class={{if this.user.isAdmin \\"admin\\"}} />
         <Ui::Button @text=\\"Click me\\" />
         <SomePath::AnotherPath::SuperSelect @selected={{this.user.country}} as |s|>
           {{#each this.availableCountries as |country|}}
@@ -565,7 +568,7 @@ test('sample', () => {
 
   expect(runTest('sample.hbs', input)).toMatchInlineSnapshot(`
     "
-        <SiteHeader @user={{this.user}} class={{if this.user.isAdmin \\"admin\\"}} />
+        <SiteHeader @user={{this.user}} @class={{if this.user.isAdmin \\"admin\\"}} />
         <SiteHeader @user={{null}} @address={{undefined}} />
 
         <SuperSelect @selected={{this.user.country}} as |s|>
@@ -888,3 +891,19 @@ test('custom-options', () => {
       "
   `);
 });
+
+test('preserve arguments', () => {
+  let input = `
+    {{foo-bar class="baz"}}
+    {{foo-bar data-baz class="baz"}}
+    {{link-to (t "show") "flight" event.flight.id class="btn btn-default btn-sm pull-right"}}
+  `;
+
+  expect(runTest('preserve-arguments.hbs', input)).toMatchInlineSnapshot(`
+    "
+        <FooBar @class=\\"baz\\" />
+        {{foo-bar data-baz class=\\"baz\\"}}
+        <LinkTo @route=\\"flight\\" @model={{event.flight.id}} class=\\"btn btn-default btn-sm pull-right\\">{{t \\"show\\"}}</LinkTo>
+      "
+  `);
+});