[Glimmer2] Migrating hash helper #13093

chadhietala · 2016-03-12T18:58:38Z

This migrate the hash helper to glimmer2 and moves tests over as well. As a result I remove a contextual-component flag that was still in the hash tests. This also adds tests for yielding a hash which was not covered at all 😄 /cc @chancancode

chancancode · 2016-03-12T19:05:36Z

packages/ember-glimmer/tests/integration/helpers/hash-test.js

+
+    this.runTask(() => set(this.context, 'firstName', 'Sergio'));
+
+    this.assertText('Sergio Arbeo');


Should test resetting to Marisa after this

chancancode · 2016-03-12T19:10:33Z

I think we probably want to leave the htmlbars implementation alone (i.e. duplicating the code, instead of symlinking). The final implementation of the glimmer version will probably be a bit different.

(I think we ultimately want to implement it as an "internal" helper like inline if/unless, where the helper gets the EvaluatedArgs instead of the values, so we can avoid re-ifying the keys that are not actually used.)

chancancode · 2016-03-12T19:12:28Z

packages/ember-glimmer/tests/integration/helpers/hash-test.js

+
+    this.assertText('Balint');
+  }
+});


New line between L89/90 to balance L6

chancancode · 2016-03-12T19:14:25Z

Other than those line comments and the symlink issue, looks great to me! (We don't have to do the internal helper optimization now, unless you want to.)

chadhietala · 2016-03-12T19:48:57Z

packages/ember-htmlbars/tests/helpers/hash_test.js

@@ -7,67 +7,65 @@ import { runAppend, runDestroy } from 'ember-runtime/tests/utils';

 var view;

-if (isEnabled('ember-contextual-components')) {


This was introduced in 2.3.0. The rest of changes in this file are just formatting.

chancancode · 2016-03-12T19:50:25Z

@chadhietala sorry :/ I was only saying that we should keep the implementation separate, what you did with the tests before was just fine

chancancode · 2016-03-12T19:51:19Z

packages/ember-htmlbars/lib/helpers/hash.js

@@ -32,4 +27,4 @@

 export default function hashHelper(params, hash, options) {
  return hash;
-}
+}


ProTip™: https://github.com/SamVerschueren/vscode-final-newline

chadhietala · 2016-03-12T19:52:21Z

@chancancode Won't we loose the test coverage there for some period time then? Maybe I don't understand.

chancancode · 2016-03-12T20:07:37Z

packages/ember-glimmer/lib/helpers/hash.js

+  return hash;
+}
+
+export default helper(hash);


https://github.com/SamVerschueren/vscode-final-newline

There is a line here. Probably a github issue.

chancancode · 2016-03-12T20:15:11Z

packages/ember-glimmer/tests/integration/helpers/hash-test.js

+  }
+
+  ['@test should yield hash of external properties']() {
+


This migrate the hash helper to glimmer2 and moves tests over as well. As a result I remove a contextual-component flag that was still in the hash tests. I also added tests for yielding a hash which was not covered by any tests.

[Glimmer2] Migrating hash helper

homu · 2016-03-12T21:07:41Z

☔ The latest upstream changes (presumably #13093) made this pull request unmergeable. Please resolve the merge conflicts.

chancancode · 2016-03-12T21:07:41Z

Merged in 0e8e343