[Glimmer2] inline if #12920

GavinJoyce · 2016-02-07T00:30:30Z

part of the Glimmer Big-Picture Integration Checklist

TODO:

mixonic · 2016-02-07T00:44:03Z

packages/ember-glimmer/lib/helpers/inline-if.js

@@ -0,0 +1,11 @@
+import { toBool as emberToBool } from './if-unless';
+
+//TODO: GJ: docs


@rwjblue says the docs for if should be moved to this file.

And thanks for noting this. In 1.13 we lost a ton of docs b/c PRs were merged without this stuff. Definitely worth doing up front.

thanks, I'll do that

homu · 2016-02-08T00:36:25Z

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

GavinJoyce · 2016-02-10T13:55:54Z

I'm going to work on this later tonight and hopefully bring it to completion

GavinJoyce · 2016-02-10T21:55:17Z

There are a number of shared tests which fail in htmlbars. For example, 6 of these fail (related issue):

moduleFor('Helpers test: {{if}} used with another helper', class extends SharedHelperConditionalsTest {
  templateFor({ cond, truthy, falsy }) {
    return `{{concat (if ${cond} ${truthy} ${falsy})}}`;
  }
});

I want to avoid marking these shared tests with a @glimmer prefix as that would result in them not being run for htmlbars for all other modules. Instead, I'm planning to add support for @glimmer and @htmlbars module prefixes. This would allow us skip these tests in htmlbars:

moduleFor('@glimmer Helpers test: {{if}} used with another helper', class extends SharedHelperConditionalsTest {
  templateFor({ cond, truthy, falsy }) {
    return `{{concat (if ${cond} ${truthy} ${falsy})}}`;
  }
});

GavinJoyce · 2016-02-10T23:00:14Z

I'm seeing 14 test failures for both @htmlbars and @glimmer for the following set of tests:

moduleFor('@glimmer Helpers test: {{if}} used in attribute position', class extends SharedHelperConditionalsTest {
  templateFor({ cond, truthy, falsy }) {
    return `<div data-foo="{{if ${cond} ${truthy} ${falsy}}}" />`;
  }
  textValue() {
    return jQuery('div', this.element).toArray().map(
      function(el) { return jQuery(el).data('foo'); }
    ).join('');
  }
});

an example failing test is [glimmer] Helpers test: {{if}} used in attribute position: it considers empty arrays falsy. Do we expect these to pass for attributes? I'm going to take try create a failing test in glimmer2 now

chancancode · 2016-02-10T23:18:01Z

@GavinJoyce I think the problem is you didn't quote the "truthy" and "falsy" properly: it currently something like <div data-foo="{{if cond1 T1 F1}}" /> (which looks up T1 and F1 as properties on self) instead of <div data-foo="{{if cond1 'T1' 'F1'}}" />.

By the way, I think it will be better to add wrapperFor so that we can generate <div data-foo="{{if cond1 'T1' 'F1'}}{{if cond2 'T2' 'F2'}}{{if cond3 'T3' 'F3'}}" /> instead of <div data-foo="{{if cond1 'T1' 'F1'}}" /><div data-foo="{{if cond2 'T2' 'F2'}}" /><div data-foo="{{if cond3 'T3' 'F3'}}" />.

GavinJoyce · 2016-02-10T23:56:30Z

@chancancode thanks, I'll implement the wrapperFor as you suggest.

I don't think it is a quoting problem, the renderValues function that I defined in SharedHelperConditionalsTest generates a template and context as follows:

<div data-foo="{{if cond1 t1 f1}}" /><div data-foo="{{if cond2 t2 f2}}" />
{"t1":"T1","f1":"F1","cond1":["hello"],"t2":"T2","f2":"F2","cond2":[]}

I can't immediately see why this isn't working, but I'm pretty sure I'm doing something wrong. I'll change it to use wrapperFor, the problem will likely go away.

chancancode · 2016-02-10T23:57:39Z

Ah, you are right! I forgot that we changed it to be dynamic

GavinJoyce · 2016-02-11T00:40:18Z

TODO: figure out why this passes:

moduleFor('Helpers test: {{if}} used in attribute position', class extends SharedHelperConditionalsTest {
  wrapperFor(templates) {
    return `<div>${templates.join('')}</div>`;
  }

  templateFor({ cond, truthy, falsy }) {
    return `{{if ${cond} ${truthy} ${falsy}}}`;
  }

  textValue() {
    return jQuery('div', this.element).text();
  }
});

but this doesn't:

moduleFor('Helpers test: {{if}} used in attribute position', class extends SharedHelperConditionalsTest {
  wrapperFor(templates) {
    return `<div data-foo="${templates.join('')}" />`;
  }

  templateFor({ cond, truthy, falsy }) {
    return `{{if ${cond} ${truthy} ${falsy}}}`;
  }

  textValue() {
    return jQuery('div', this.element).data('foo');
  }
});

GavinJoyce · 2016-02-11T15:16:14Z

@paddyobrien solved this for me. It turns out that jQuery caches the data attribute values when accessing the values using:

jQuery('div', this.element).data('foo');

This solves it:

jQuery('div', this.element).attr('data-foo');

chancancode · 2016-02-11T17:25:29Z

Ahh I see. It's probably good to use attr anyway so jQuery doesn't try to parse the text

GavinJoyce · 2016-02-11T17:44:05Z

This is ready for review

rwjblue · 2016-02-11T18:48:36Z

@chancancode - Would you mind reviewing?

chancancode · 2016-02-11T21:17:45Z

@GavinJoyce can you do me a favor and annotate the deleted test case with comments like these? #12914 (comment) We are trying to be careful to not lose coverage as we migrate these, and having those annotations would make it a lot easier to review! ❤️

The tests doesn't always have to be a direct port. We implicitly test a lot more things with the new harness/pattern, so it is quite possible we absorbed some existing test cases into other test cases implicitly (e.g. we used to have separate test cases for re-render, but we now test re-render all the time, so it makes sense to just delete it).

GavinJoyce · 2016-02-11T21:22:35Z

@chancancode sure, I'll do that now

GavinJoyce · 2016-02-11T21:23:29Z

packages/ember-glimmer/tests/integration/helpers/inline-if-test.js

+
+}, BASIC_TRUTHY_TESTS, BASIC_FALSY_TESTS);
+
+moduleFor('@glimmer Helpers test: {{if}} used with another helper', class extends SharedHelperConditionalsTest {


@glimmer only as several of these tests fail in @htmlbars (see #12925)

GavinJoyce · 2016-02-11T21:40:38Z

@chancancode I've added a SharedHelperConditionalsTest version of the @htmlbars it does not update when the unbound helper is used test, as it needs a different template.

I've also added annotations for the two modules which are @glimmer only. I haven't removed any tests, the diff might look like it due to a few chunks of content moving around though.

GavinJoyce · 2016-02-11T21:44:47Z

packages/ember-glimmer/tests/utils/abstract-test-case.js

-  QUnit.module(`[${packageName}] ${description}`, {
+  let modulePackagePrefixMatch = description.match(/^@(\w*)/); //eg '@glimmer' or '@htmlbars'
+  let modulePackagePrefix = modulePackagePrefixMatch ? modulePackagePrefixMatch[1] : '';
+  let descriptionWithoutPackagePrefix = description.replace(/^@\w* /, '');


this adds support for module prefixes such as @glimmer and @htmlbars and is needed as the new comprehensive shared tests have highlighted some issues with htmlbars

GavinJoyce · 2016-02-11T21:50:03Z

Let me know if you'd like me to annotate anything else

chancancode · 2016-02-11T22:59:52Z

@GavinJoyce sorry! I should have looked more carefully – I copied the comment from here assuming it would make sense.

Can you review the existing tests and either delete (with the annotation) them or migrate them into the new test files? If some of them are too hard to port we can leave them in the old file and come back to them later; but I suspect your work already covered the majority of them, so seems good to have you review and delete them while your memory is still fresh.

Great work so far by the way, thank you so much for picking this up!!

GavinJoyce · 2016-02-11T23:07:01Z

Ah, thanks for the clarification. Sure, I'll do that on Saturday which will be the next time I'll get to work on this.

I'm happy to continue working on glimmer2 integration once this has merged too, we're very excited to begin using it Intercom

chancancode · 2016-02-17T19:04:00Z

packages/ember-glimmer/lib/helpers/inline-if.js

+    return args[1];
+  } else {
+    let falsyArgument = args[2];
+    return falsyArgument === undefined ? '' : falsyArgument;


I suppose this is needed to pass the tests at the moment? Can you add a TODO here? I believe we should unconditional return args[2] here (even if it's undefined) and let a different part of the system handle converting undefined, null, etc to "" (which we haven't implemented yet).

It's probably good to add a test for this case if we don't already have one (I think it would have to be @htmlbars for now), but perhaps that should be part of the general checklist item of "Add tests for rending false, undefined, null etc in content-test.js, similar to..."

this.render(`{{if condition truthy}}`, { condition: true, truthy: null }); this.render(`{{if condition truthy}}`, { condition: true, truthy: undefined }); this.render(`{{if condition truthy}}`, { condition: true, truthy: false }); ...etc

yes, this was added to allow the moved it can omit the falsy argument test to pass. I'll modify this test to be @htmlbars and add a TODO as you suggest.

Seems like this got lost in the merge too, we retained the TODO comment but lost the @htmlbars marker

GavinJoyce · 2016-02-17T20:52:11Z

@chancancode thanks for your feedback. I've made the following changes:

added comment : https://github.com/emberjs/ember.js/pull/12920/files#diff-8bf5a47bad15668bbc3a067fefd4118dR35
made related test @htmlbars: https://github.com/emberjs/ember.js/pull/12920/files#diff-034be756e33abf234e1e3ff8f648e810R18
removed the it allows static arguments test which was covered elsewhere
ensured that all remaining tests have the Reset (via replacement if applicable) step

chancancode · 2016-02-17T22:34:21Z

packages/ember-glimmer/tests/integration/helpers/inline-if-test.js

+    this.runTask(() => set(this.context, 'condition', true));
+
+    this.assertText('yes');
+  }


👍

We should test both starting conditions (starting with true and starting with false), like this: https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/tests/integration/syntax/if-unless-test.js#L52-L79

Thanks, done

chancancode · 2016-02-17T23:10:51Z

I ran out of time for this review session, I'll return to it another time! But I have left some "big picture" comments for you to chew on at the moment!

GavinJoyce · 2016-02-17T23:14:29Z

Thanks for your feedback @chancancode. I'm keen to nail these testing patterns early so that the we can quickly and comprehensively integrate the rest of glimmer2 into ember

chancancode · 2016-02-18T18:27:56Z

packages/ember-glimmer/tests/integration/helpers/inline-if-test.js

+    this.assertText('falsy');
+  }
+
+  ['@test it updates when given a falsey second argument']() {


🤘 (I guess this must be a regression test?)

chancancode · 2016-02-24T19:40:45Z

packages/ember-glimmer/tests/utils/abstract-test-case.js

@@ -78,6 +86,7 @@ export class RenderingTest extends TestCase {
    let owner = this.owner = buildOwner();
    let env = this.env = new Environment({ dom, owner });
    this.renderer = new Renderer(dom, { destinedForDOM: true, env });
+    this.$ = jQuery;


ah, this is not what I meant, I mean something like how this.$() works in components, but I can fix it

[Glimmer2] inline if

Merge + minor cleanup for #12920

chancancode · 2016-02-24T22:11:42Z

This is really awesome! Thanks for taking the time to work on this @GavinJoyce ❤️ 💚 💛 💙 💜

mixonic reviewed Feb 7, 2016
View reviewed changes

This was referenced Feb 7, 2016

inline {{if}} helper doesn't update when used within another helper #12923

Closed

inline {{if}} helper within a sub expression doesn't respect array updates #12925

Closed

GavinJoyce changed the title ~~[wip] glimmer inline if~~ [wip] glimmer2 inline if Feb 10, 2016

GavinJoyce changed the title ~~[wip] glimmer2 inline if~~ Glimmer2 inline if Feb 11, 2016

GavinJoyce reviewed Feb 11, 2016
View reviewed changes

chancancode mentioned this pull request Feb 17, 2016

Glimmer Big-Picture Integration Checklist #12907

Closed

52 tasks

chancancode reviewed Feb 17, 2016
View reviewed changes

chancancode reviewed Feb 18, 2016
View reviewed changes

Glimmer2 inline if

7c0befb

chancancode reviewed Feb 24, 2016
View reviewed changes

chancancode added a commit that referenced this pull request Feb 24, 2016

Merge pull request #12920 from GavinJoyce/gj/glimmer-inline-if

527ba41

[Glimmer2] inline if

chancancode merged commit 7c0befb into emberjs:master Feb 24, 2016

chancancode added a commit that referenced this pull request Feb 24, 2016

Merge pull request #13009 from emberjs/merge-12920

1659f53

Merge + minor cleanup for #12920

GavinJoyce deleted the gj/glimmer-inline-if branch February 24, 2016 22:16

GavinJoyce mentioned this pull request Feb 26, 2016

Glimmer2 inline {{unless}} #13019

Closed

2 tasks

chancancode mentioned this pull request Mar 19, 2016

[Pls send halp!] The Ultimate Glimmer 2 Test Porting Guide #13127

Closed

45 tasks

		@@ -0,0 +1,11 @@
		import { toBool as emberToBool } from './if-unless';

		//TODO: GJ: docs


		}, BASIC_TRUTHY_TESTS, BASIC_FALSY_TESTS);

		moduleFor('@glimmer Helpers test: {{if}} used with another helper', class extends SharedHelperConditionalsTest {

[Glimmer2] inline if #12920

[Glimmer2] inline if #12920

Conversation

GavinJoyce commented Feb 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homu commented Feb 8, 2016

GavinJoyce commented Feb 10, 2016

GavinJoyce commented Feb 10, 2016

GavinJoyce commented Feb 10, 2016

chancancode commented Feb 10, 2016

GavinJoyce commented Feb 10, 2016

chancancode commented Feb 10, 2016

GavinJoyce commented Feb 11, 2016

GavinJoyce commented Feb 11, 2016

chancancode commented Feb 11, 2016

GavinJoyce commented Feb 11, 2016

rwjblue commented Feb 11, 2016

chancancode commented Feb 11, 2016

GavinJoyce commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinJoyce commented Feb 11, 2016

Choose a reason for hiding this comment

GavinJoyce commented Feb 11, 2016

chancancode commented Feb 11, 2016

GavinJoyce commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinJoyce commented Feb 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancancode commented Feb 17, 2016

GavinJoyce commented Feb 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancancode commented Feb 24, 2016