Skip to content

Implement Ember semantics for {{#if}}/{{#unless}}#12914

Merged
rwjblue merged 2 commits intoemberjs:masterfrom
tildeio:if-unless-tests
Feb 6, 2016
Merged

Implement Ember semantics for {{#if}}/{{#unless}}#12914
rwjblue merged 2 commits intoemberjs:masterfrom
tildeio:if-unless-tests

Conversation

@chancancode
Copy link
Member

Also ported most of the basic test cases to the new format. I expect the super classes here can be extracted into a different file and we should be able to reuse all the test cases for the inline (if …) and (unless …) helpers as well, because of the test cases go through the templateFor abstraction.

I have not figured out a good way to abstract the “without inverse” tests, so they are duplicated in the leaf subclasses.

The rest of the tests in the if_unless_test requires some more careful reading/git-history-digging to see if they are still relevant, which I do not have time for at the moment (maybe someone else can pick up where I left off).

In any case, implementing the Ember semantics should be enough to unblock porting other tests that depends on them. I did not implement the inline (if …) and (unless …) helpers or port their tests, but that should be pretty easy given the {{concat}} example and the toBool function in here.

Note: please don’t add the inline (if …) and (unless …) tests here. Instead, please add them to integration/helpers/if-unless-test.js (and extract the shared test cases to a different file too).

cc @wycats @rwjblue @mmun @Serabe

@chancancode
Copy link
Member Author

For the inline helpers, we can potentially subclass multiple times for test the same helpers in different positions. Something like:

moduleFor('Helpers test: {{if}} used in content position', class extends SharedConditionalsTest {

  wrapperFor(templates) {
    return templates.join('');
  }

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

  get result() {
    jQuery(this.element).text();
  }

});

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

  wrapperFor(templates) {
    return `{{concat ${templates.join(' ')}}}`;
  }

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

  get result() {
    jQuery(this.element).text();
  }

});

moduleFor('Helpers test: {{if}} used in attribute position', class extends SharedConditionalsTest {

  wrapperFor(templates) {
    return `<div data-foo="${templates.join('')}" />`;
  }

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

  get result() {
    jQuery('div', this.element).data('foo');
  }

});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should probably be moved up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced by:

  • "it tests for isTruthy if available"
  • "it tests for isTruthy on Ember objects if available"

(Now testing re-render, mutation and replacement as well)

@chancancode
Copy link
Member Author

I think this is good to merge regardless of how we feel about my comments – if we feel that we are missing any coverage I am happy to revert removing those particular tests (keeping them in the old tests) and come back to it another day.

@chancancode
Copy link
Member Author

After merging this, we should unskip https://github.com/emberjs/ember.js/pull/12910/files#diff-143647b81033de48d2908310dcce5644R46 for glimmer once we implemented user helpers lookup.

Also ported most of the basic test cases to the new format. I expect the
super classes here can be extracted into a different file and we should
be able to reuse all the test cases for the inline (if …) and (unless …)
helpers as well, because of the test cases go through the `templateFor`
abstraction.

I have not figured out a good way to abstract the “without inverse”
tests, so they are duplicated in the leaf subclasses.

The rest of the tests in the `if_unless_test` requires some more careful
reading/git-history-digging to see if they are still relevant, which I
do not have time for at the moment (maybe someone else can pick up where
I left off).

In any case, implementing the Ember semantics should be enough to
unblock porting other tests that depends on them. I did not implement
the inline `(if …)` and `(unless …)` helpers or port their tests, but
that should be pretty easy given the `{{concat}}` example and the
`toBool` function in here.

Note: please don’t add the inline `(if …)` and `(unless …)` tests here.
Instead, please add them to `integration/helpers/if-unless-test.js` (and
extract the shared test cases to a different file too).
@rwjblue
Copy link
Member

rwjblue commented Feb 6, 2016

Thanks for the extra comments. Made review much easier.

We need to force a rerender (similar to `scheduleRevalidation`) for
glimmer because we don’t have runloop integration yet, but we intend to
remove that soon. However, we should not force the same for htmlbars so
we don’t accidentally regress functionality.

Also, rename `inZone` to `runTask`.
rwjblue added a commit that referenced this pull request Feb 6, 2016
Implement Ember semantics for {{#if}}/{{#unless}}
@rwjblue rwjblue merged commit ddd1afb into emberjs:master Feb 6, 2016
@homu homu mentioned this pull request Feb 6, 2016
3 tasks
@GavinJoyce GavinJoyce mentioned this pull request Feb 7, 2016
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants