[BUGFIX release] Fixes {{#with proxy as |foo|}}

[chancancode]

This fixes an issue in HTMLBars where passing a truthy proxy (i.e. { isTruthy: true, ... }) to {{#with}} would end up yielding the literal true rather than the proxy itself.

The Glimmer implementation is unaffected by this bug.

Fixes #13045

[chancancode]

Should I use a real proxy here? My concern is that we will remove the actual proxy implementations when the the addons get EOL'ed, and when we do that, we might end up removing this test.

(Actually, are the isTruthy stuff intrinsically important, or are they just there to help us implement proxies? i.e. can we remove support for isTruthy when we remove proxies?)

[chancancode]

actually, I was confused – controllers were being removed but not proxies, so, ignore me!

[chancancode]

I am not very familiar with this code, but if we are already replacing params[0], why are we calling shouldDisplay again here?

Also, the second shouldDisplay is not the same as the first shouldDisplay. However, I don't think it's doing anything – params[0] here must already be the coercer-ed value from link-render-node, otherwise we wouldn't be having this bug? (The same is true for if/unless)

I couldn't quite figure it out, could use 👀 from someone who knows the code better than me. I think we probably have another bug here somewhere – at minimum I suspect we might be doing unnecessary (and potentially expensive) work for each if/unless/with block.

@mmun @rwjblue r?

[rwjblue]

A similar issue was fixed in #12996 (which was hit when you didn't have isTruthy), but that fix didn't use the coercer(which I think it should). Can you double check, and update that here also if you agree?

[chancancode]

Added the coercer for arrays. As far as I can tell, that does not (and cannot) make any difference in practice.

However, I am still quite bothered by the double shouldDisplay call. If the link-render-node shouldDisplay is in charge of the check (and also coercering the value), then we shouldn't need to do another (and different!) shouldDisplay check (from ember-views/streams/should_display) when rendering. Something like this:

diff --git a/packages/ember-htmlbars/lib/helpers/if_unless.js b/packages/ember-htmlbars/lib/helpers/if_unless.js
index 2b018f0..a82954c 100644
--- a/packages/ember-htmlbars/lib/helpers/if_unless.js
+++ b/packages/ember-htmlbars/lib/helpers/if_unless.js
@@ -4,7 +4,6 @@
 */

 import { assert } from 'ember-metal/debug';
-import shouldDisplay from 'ember-views/streams/should_display';

 /**
   Use the `if` block helper to conditionally render a block depending on a
@@ -63,7 +62,7 @@ import shouldDisplay from 'ember-views/streams/should_display';
   @public
 */
 function ifHelper(params, hash, options) {
-  return ifUnless(params, hash, options, shouldDisplay(params[0]));
+  return ifUnless(params, hash, options, params[0] !== false);
 }

 /**
@@ -76,7 +75,7 @@ function ifHelper(params, hash, options) {
   @public
 */
 function unlessHelper(params, hash, options) {
-  return ifUnless(params, hash, options, !shouldDisplay(params[0]));
+  return ifUnless(params, hash, options, params[0] === false);
 }

 function ifUnless(params, hash, options, truthy) {
diff --git a/packages/ember-htmlbars/lib/helpers/with.js b/packages/ember-htmlbars/lib/helpers/with.js
index 0bfa9c3..84bd5cd 100644
--- a/packages/ember-htmlbars/lib/helpers/with.js
+++ b/packages/ember-htmlbars/lib/helpers/with.js
@@ -3,8 +3,6 @@
 @submodule ember-templates
 */

-import shouldDisplay from 'ember-views/streams/should_display';
-
 /**
   Use the `{{with}}` helper when you want to alias a property to a new name. This is helpful
   for semantic clarity as it allows you to retain default scope or to reference a property from another
@@ -39,7 +37,7 @@ import shouldDisplay from 'ember-views/streams/should_display';
 */

 export default function withHelper(params, hash, options) {
-  if (shouldDisplay(params[0])) {
+  if (params[0] !== false) {
     options.template.yield([params[0]]);
   } else if (options.inverse && options.inverse.yield) {
     options.inverse.yield([]);
diff --git a/packages/ember-htmlbars/lib/hooks/link-render-node.js b/packages/ember-htmlbars/lib/hooks/link-render-node.js
index dd661f3..9bf9fba 100644
--- a/packages/ember-htmlbars/lib/hooks/link-render-node.js
+++ b/packages/ember-htmlbars/lib/hooks/link-render-node.js
@@ -94,7 +94,7 @@ function shouldDisplay(predicate, coercer) {
       return isTruthyVal ? coercer(predicateVal) : false;
     }

-    return coercer(predicateVal);
+    return predicateVal ? coercer(predicateVal) : false;
   }, 'ShouldDisplay');

   addDependency(stream, length);

But when I tried that, I started running into bugs for cases where link-render-node is not called, e.g. in attribute helpers positions, which is probably the same reason why these tests are not working.

I agree that what I have here is the most conservative fix for the reported problem, for the purpose of back-porting. However, I think we are currently just masking another bug by calling shouldDisplay twice (two different ones!). :-/

[rwjblue]

Agreed. I think the best course of action is to squash this bug and stop trying to plug the holes in the dam (and instead focus on landing glimmer v2)...

[chancancode]

👍

[chancancode]

(I think the fix I have here should be sufficient, but let me know if you believe otherwise)