Glimmer2 inline {{unless}}

[GavinJoyce]

follow on from #12920
part of the Glimmer Big-Picture Integration Checklist

TODO:

• rebase
• use this technique

[chancancode]

We could write export function inlineIf(

[GavinJoyce]

thanks

[GavinJoyce]

@chancancode Do you think there is value in repeating all the existing inline if tests for inline unless? (I'm currently just testing the main use, outlined below) If so, any ideas on how I might avoid simply duplicating them?

moduleFor('Helpers test: inline {{unless}}', class extends SharedHelperConditionalsTest {

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

}, BASIC_TRUTHY_TESTS, BASIC_FALSY_TESTS);

[chancancode]

@GavinJoyce yeah, I think we should test them. I think we mostly did the work to make them fairly reusable (by "duplicating" a few lines we get 20+ tests).

I think there are a few kinds of scenarios:

• If there is a way we can share them between inline if and inline unless, then we should put that in a super class they share.
• I couldn't figure out how to share some them – specifically the "without inverse" test. If you have ideas I would love to see it, but if not I think it's fine to duplicate them (it's just one or a few test cases iirc, and we do that for the syntax tests too).
• Probably the most The mixin/generator thing (BASIC_*_TESTS) are duplicated everywhere which is a bit annoying. Ideally you should always get them when you subclass from SharedConditionalsTests. This is kind of a problem with using ES6 classes since they don't have mixins. We can perhaps extract the mixin logic from moduleFor into a separate function, which allows us to apply them to SharedConditionalsTests, then all subclass would get them. (If you don't want to take on that right now, I think it's fine to keep duplicating too.)

[chancancode]

@GavinJoyce in your specific example:

moduleFor('Helpers test: inline {{unless}}', class extends SharedHelperConditionalsTest {

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

}, BASIC_TRUTHY_TESTS, BASIC_FALSY_TESTS);

I think this level of duplication is acceptable. The main boilerplate here is the BASIC_TRUTHY_TESTS, BASIC_FALSY_TESTS part, which we can either solve by pre-mixing them into SharedConditionalTest or by making a moduleForConditionals.

Besides that, I think all the code here are actually saying something meaningful. (I probably would prefer not having to do the moduleFor dance, but the only way I know is to use class decorators.)

[GavinJoyce]

Thanks, I'll spend some time tomorrow trying to make the tests as succinct as possible

[chancancode]

🤘

[chancancode]

Of course, if only difference is just the string if vs unless, we can always do...

["if", "unless"].forEach(theWord => {

  moduleFor(`...${theWord}...`, class ...);

  moduleFor(`...${theWord}...`, class ...);

});

I am 50-50 on these kinds of DRY-ing refactor. IMO this makes things much harder to read/reason about and usually isn't worth it when the amount of duplication is not that high. I think when the factor is just 2 (if, unless), the repetition is cheaper than the extra cognitive overhead, but I will probably be happy with whatever you decided is better.

[homu]

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

[GavinJoyce]

it would be nice to provide different asserting messages for {{if}} and {{unless}} but having a generic message now allows the tests to be compatible with HTMLBars. Perhaps I should add a TODO?

[GavinJoyce]

@chancancode this is ready for review.

I didn't use ["if", "unless"].forEach(... as in most cases the truthy and falsy arguments also needed to be swapped.

I used your "without inverse" trick for Helpers test: inline {{if}} and {{unless}} without the inverse argument, thanks for that.

[homu]

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

[chancancode]

Rebased and merged in #13092, thanks @GavinJoyce!