New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw error if value used with `#with` is falsy #770

Closed
matb33 opened this Issue Mar 2, 2013 · 11 comments

Comments

Projects
None yet
7 participants
@matb33

matb33 commented Mar 2, 2013

Using the #with helper that resolves to a falsy value can lead to very strange results that are quite difficult to debug. I suggest Meteor do a check that makes sure that what is being passed to #with is not falsy.

An example to reproduce the strangeness:

<body>
    {{> index}}
</body>

<template name="index">
    {{#with documentPass}}
        <button {{#if disabled}}disabled{{/if}}>Test Button (Pass)</button>
    {{/with}}
    {{#with documentFail}}
        <button {{#if disabled}}disabled{{/if}}>Test Button (Fail)</button>
    {{/with}}
</template>
var Test = new Meteor.Collection("test");

if (Meteor.isClient) {
    Session.set("id", false);   // simulate id not being set yet

    Template.index.documentPass = function () {
        var id = Session.get("id");
        return Test.findOne({_id: Session.get("id")});
    };
    Template.index.documentFail = function () {
        var id = Session.get("id");
        return id && Test.findOne({_id: Session.get("id")});
    };
    Template.index.disabled = function () {
        return true;
    };
}
@belisarius222

This comment has been minimized.

belisarius222 commented Mar 3, 2013

I've also encountered problems with this.

I wonder if there would be a significant downside to having the {{#with}}
block helper take an optional {{else}} block, just like {{#if}} and
{{#each}}. That way if the argument to {{#with}} is falsy, it will display
the contents of the else block. If you haven't specified the else block,
it just wouldn't render the template.

I think this behavior would parallel the other default block helpers in a
way that's useful and feels consistent. If there's some consensus around
this, I'd be down for submitting a pull request.

On Sat, Mar 2, 2013 at 6:00 PM, Mathieu Bouchard
notifications@github.comwrote:

Using the #with helper that resolves to a falsy value can lead to very
strange results that are quite difficult to debug. I suggest Meteor do a
check that makes sure that what is being passed to #with is not falsy.

An example to reproduce the strangeness:

{{> index}} {{#with documentPass}} Test Button (Pass) {{/with}} {{#with documentFail}} Test Button (Fail) {{/with}}

var Test = new Meteor.Collection("test");
if (Meteor.isClient) {
Session.set("id", false); // simulate id not being set yet

Template.index.documentPass = function () {
    var id = Session.get("id");
    return Test.findOne({_id: Session.get("id")});
};
Template.index.documentFail = function () {
    var id = Session.get("id");
    return id && Test.findOne({_id: Session.get("id")});
};
Template.index.disabled = function () {
    return true;
};}


Reply to this email directly or view it on GitHubhttps://github.com//issues/770
.

@matb33

This comment has been minimized.

matb33 commented Mar 3, 2013

@belisarius222 I agree with your suggestion

@gschmidt

This comment has been minimized.

Member

gschmidt commented Mar 5, 2013

@belisarius222, @glasser and I like your proposal too. 👍 We'd take a pull request, especially if it included a test and a suggested addition to http://github.com/meteor/meteor/wiki/Handlebars. The relevant code is in packages/handlebars/evaluate.js -- I think you just need to add another line or two to 'with' on line 30 following the model of 'if' on line 51.

@belisarius222

This comment has been minimized.

belisarius222 commented Mar 6, 2013

Cool! Thanks for pointing me in the right direction. I'll keep you posted.

On Tue, Mar 5, 2013 at 12:00 AM, gschmidt notifications@github.com wrote:

@belisarius222 https://github.com/belisarius222, @glasserhttps://github.com/glasserand I like your proposal too. [image:
👍] We'd take a pull request, especially if it included a test and a
suggested addition to http://github.com/meteor/meteor/wiki/Handlebarshttps://github.com/meteor/meteor/wiki/Handlebars.
The relevant code is in packages/handlebars/evaluate.js -- I think you just
need to add another line or two to 'with' on line 30 following the model of
'if' on line 51.


Reply to this email directly or view it on GitHubhttps://github.com//issues/770#issuecomment-14423602
.

estark37 added a commit to estark37/meteor that referenced this issue Mar 22, 2013

estark37 added a commit to estark37/meteor that referenced this issue Mar 22, 2013

Fix meteor#770.
Add optional else case to {{#with}}.
@estark37

This comment has been minimized.

Contributor

estark37 commented Mar 22, 2013

Sorry if I'm stepping on your toes, @belisarius222! I was looking for something simple to get started on.

glasser added a commit that referenced this issue Mar 22, 2013

glasser added a commit that referenced this issue Mar 22, 2013

Fix #770.
Add optional else case to {{#with}}.

@glasser glasser closed this Mar 22, 2013

sdarnell referenced this issue in sdarnell/meteor-win Apr 7, 2013

sdarnell referenced this issue in sdarnell/meteor-win Apr 7, 2013

Fix #770.
Add optional else case to {{#with}}.
@alanning

This comment has been minimized.

Contributor

alanning commented May 6, 2013

I'd like to respectfully suggest re-opening this issue and reverting the change.

My reasoning for this includes the following:

  1. the prior behavior of the WITH helper matches the expected behavior as described on the Handlebars site
  2. an effective workaround exists for the OP's case: use of an enclosing IF / ELSE block
  3. no workaround currently exists for obtaining the original behavior other than defining a custom block helper or over-riding the existing helper

Using an enclosing IF / ELSE block will be familiar to anyone who has used Handlebars outside of Meteor. One example of how the original behavior of the WITH block was beneficial is the case of merging Create & Update forms.

With the original behavior, forms could be coded like this:

<form>
  {{#with person}}}
  <input id="name" value="{{name}}" />
  <input id="age" value="{{age}}" />
  {{/with}}
</form>

The form can be used for the Create case, where a person object does not already exist, as well as for the update case where a person does already exist. With the current behavior, the form contents do not display unless person exists, so duplication in a corresponding ELSE block is required.

@raix

This comment has been minimized.

Contributor

raix commented May 6, 2013

@alanning you could initialize the person object with defaults, maybe something like: return person || {};
@belisarius222 could use each instead, wrapping object in array, eg. Return [person]; gettin the else
I'm not sure how much handlebars spec should rule, the goal should be a better meteor adapted templating language. Eg. Each Will support iterating over objects, and have a {{key}}? Set, this is not hb specs but it's common it other temp languages.

@alanning

This comment has been minimized.

Contributor

alanning commented May 6, 2013

In the example case, initializing with a default person object won't work because it changes the expected behavior. Extending the example to illustrate a more real-world usage,

{{#with person}}}
<form>
  <input id="name" value="{{name}}" />
  <input id="age" value="{{age}}" />

  {{#if person}}
    <input type="submit" value="Update" />
  {{else}}
    <input type="submit" value="Create" />
  {{/if}}
</form>
{{/with}}

The workaround for the OP is to wrap the WITH block in an IF/ELSE. Reproducing the original behavior in the current code base can be achieved by overriding the WITH helper:

  Handlebars._default_helpers.with = function(context, options) {
    return options.fn(context);
  };

...but this requires messing with internals, whereas reverting to the original behavior enables both scenarios using built-in functionality.

I agree HB spec shouldn't necessarily dictate Meteor usage, but in this case I think the original behavior is more flexible.

@raix

This comment has been minimized.

Contributor

raix commented May 6, 2013

@alanning I see,
@glasser: When taking a step back - why dont behaviour of with depend on the existens of else?
When using with we kinda force the use of an object in scope, even if its a falsy?, so: If no else is defined it should behave as a normal with? but with the option of using an else
Each on the other hand depends on element since it iterates over them, so we cant force usage of empty objects here.

@matb33

This comment has been minimized.

matb33 commented Jul 24, 2013

@alanning @glasser I think we may have gotten distracted by a quick solution to a deeper issue.

@allaning Based on your example, if person is falsy, you can get garbled output in certain cases. For example:

{{#with person}}}
<form>
  <input id="name" value="{{name}}" />
  <input id="age" value="{{age}}" {{#if disableAgeField}}disabled{{/if}} />

  {{#if person}}
    <input type="submit" value="Update" />
  {{else}}
    <input type="submit" value="Create" />
  {{/if}}
</form>
{{/with}}

Note the addition of the {{#if disabledAgeField}}disabled{{/if}}. Try running this template through with a falsy person object. As of the time of the writing of my original issue, this led to garbled output. I don't recall the exact details, but I believe there were issues with the comments that Spark generated to manage landmarks.

avital added a commit that referenced this issue Nov 8, 2013

Refix issue #770
{{#with falseyValue}} should render {{else}} content
@matb33

This comment has been minimized.

matb33 commented Dec 3, 2013

I've been using #with with its new behavior for quite a while now, and I have to say I much prefer it to the original behavior. It's especially useful in the context of Meteor with regards to invalidations causing recomputations.

Since #with is now basically an #if that also sets context, you can go from:

{{#if duration}}{{duration}} hour{{#if notequals duration 1}}s{{/if}}{{/if}}

to

{{#with duration}}{{this}} hour{{#if notequals this 1}}s{{/if}}{{/with}}

... which as far as my understanding goes, will only compute duration once instead of 3 times in the original. If duration happens to be a complex operation, the savings become that much more important.

(Note that "notequals" isn't stock Meteor -- it's a made-up Handlebars helper used to help illustrate the example)

martijnwalraven pushed a commit that referenced this issue Feb 22, 2016

martijnwalraven pushed a commit that referenced this issue Feb 22, 2016

Refix issue #770
{{#with falseyValue}} should render {{else}} content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment