Skip to content
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

Looking up in parent context on foo: null is contrary to RFC #397

Closed
Roman600 opened this issue Nov 8, 2014 · 24 comments
Closed

Looking up in parent context on foo: null is contrary to RFC #397

Roman600 opened this issue Nov 8, 2014 · 24 comments

Comments

@Roman600
Copy link

Roman600 commented Nov 8, 2014

Mustache RFC:

Variables
The most basic tag type is the variable. A {{name}} tag in a basic template will try to find the name key in the current context. If there is no name key, the parent contexts will be checked recursively. If the top context is reached and the name key is still not found, nothing will be rendered.

But I do have a property in children object and it's value is null (which is false in some way). And I expect nothing to be rendered, but instead mustache.js looks up for a value in parent context and finds it, and shows it. But it's parent's value, not children's. And children has it's own value, and it's null. So according to RFC looking up doesn't need to be done.

The same thing happens not with only variables {{key}}, but with sections {{#key}}{{/key}} too.

@Roman600 Roman600 closed this as completed Nov 8, 2014
@Roman600 Roman600 reopened this Nov 8, 2014
@billymoon
Copy link

👍 - can't find any good way around this issue myself - thinking about switching to another template engine if I can't get a solution :(

@janl
Copy link
Owner

janl commented Nov 26, 2014

Cool, good luck elsewhere. I hear handlebars is nice.

On 26.11.2014, at 01:10, Billy Moon [email protected] wrote:

  • can't find any good way around this issue myself - thinking about switching to another template engine if I can't get a solution :(


Reply to this email directly or view it on GitHub.

@bubbatls
Copy link

Handlebars is nice but it does not check parent context at all.
If I'd like to do what you need I'll try something like that

Object.prototype.noupcontext = function () {
  var me = this;
  return function(val, render) {
    return me[render(val)];
  };
};

and {{#noupcontext}}name{{/noupcontext}}

@janl
Copy link
Owner

janl commented Nov 26, 2014

I was just commenting on your subtle thread to leave mustache if your problem isn’t addressed. This is a volunteer project and people threatening the volunteers is not something I’d like to see in this project. Good luck elsewhere.

@bubbatls
Copy link

who are you talking to? I was just helping

@janl
Copy link
Owner

janl commented Nov 26, 2014

I’m referring to @billymoon.

@Roman600
Copy link
Author

Just for using in my project in Context.prototype.lookup function I changed

        if (value != null)
          break;

to

        if (value !== undefined)
          break;

Anyone can tell why it could be wrong?

@billymoon
Copy link

@Roman600 I think value != null is functionally equivalent to value !== null && value !== undefined

@billymoon
Copy link

@bubbatls I checked your method, and it works. I will need to make some tweaks to fit my case exactly (how to include strings not in the data when the element exists in the data, and hopefully implement without changing Object.prototype) but I think it will be a working solution. I will post a fiddle here once I am done with my tweaks. Thanks!

@bubbatls
Copy link

@Roman600 solution seams far better than mine. In my mind it can be wrong only for the thousands (millions ?) existing templates around the world that expect null to be handled as undefined.
But as the Context is public you might override the method without modifying mustache.js itself

@dasilvacontin
Copy link
Collaborator

See PR #407.

@dasilvacontin
Copy link
Collaborator

This is a bug since, as @Roman600 mentioned, there is actually a key. Specially for the case the key has null as value. From MDN:

The value null is a JavaScript literal representing null or an "empty" value, i.e. no object value is present.

null is probably the best way to indicate that there's no value for the key.

For keys having undefined as value, we could check them using .hasOwnProperty(key). Strictly according to the spec, if the key exists, even if it has the value undefined, the parent contexts shouldn't be checked. However, in this case, I'm concerned about a lot of people relying on keys with undefined value being ignored and checking the parent contexts anyways.

@dasilvacontin
Copy link
Collaborator

Any thoughts about not checking parent contexts if the current context has the key, regardless of it's value (even undefined)?

@bobthecow
Copy link

Changing the default here isn't something that should be taken lightly. Even if it's to move to something that makes more sense, it'll break a lot of existing apps and sites. Maybe it would make sense to add a config option for this?

@dasilvacontin
Copy link
Collaborator

@bobthecow, indeed, that's my main concern about it; even though it's something spec-compliant, I'm sure that a lot of people rely on undefined being disregarded.

About the config option, do you have any suggestion/ideas? A pragma, programatic/API, ...?

@janl
Copy link
Owner

janl commented Dec 18, 2014

or new major version, yay SemVer.

@dasilvacontin
Copy link
Collaborator

@janl, well, mustache.js is still on 0.x.x, so we would still be SemVer compliant if there were breaking changes. 😅

@janl
Copy link
Owner

janl commented Dec 18, 2014

yeah, that needs fixing too, realise the current version as 1.0.0 and move on from there :)

@dasilvacontin
Copy link
Collaborator

@janl yeah, that should happen soon :). I'd also like to get more feedback on #406 to know which direction we are headed to.

@janl
Copy link
Owner

janl commented Dec 18, 2014

What else do you want feedback on, there? :)

On 18.12.2014, at 16:40, David da Silva Contín [email protected] wrote:

@janl yeah, that should happen soon :). I'd also like to get more feedback on #406 to know which direction we are headed to.


Reply to this email directly or view it on GitHub.

@dasilvacontin
Copy link
Collaborator

@janl Solved. :)

dasilvacontin added a commit to dasilvacontin/mustache.js that referenced this issue Dec 19, 2014
dasilvacontin added a commit to dasilvacontin/mustache.js that referenced this issue Dec 19, 2014
dasilvacontin added a commit to dasilvacontin/mustache.js that referenced this issue Dec 19, 2014
@dasilvacontin
Copy link
Collaborator

A key with undefined or null value will be considered as a lookup hit in mustache.js v2. Breaking backwards compatibility.

@dasilvacontin dasilvacontin added this to the 2.0.0 milestone Dec 20, 2014
dasilvacontin added a commit to dasilvacontin/mustache.js that referenced this issue Mar 22, 2015
@phillipj
Copy link
Collaborator

phillipj commented May 5, 2015

Fixed in a020430

@phillipj phillipj closed this as completed May 5, 2015
@dasilvacontin
Copy link
Collaborator

Oh, that commit is from when I believed writing 'fixes' once before a bunch of issue numbers was enough. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants