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

Inconsistency when passing functions (e.g. descendants of can.Construct) to helpers. #450

Closed
bmomberger-reciprocity opened this Issue Jul 26, 2013 · 15 comments

Comments

Projects
None yet
4 participants
@bmomberger-reciprocity
Contributor

bmomberger-reciprocity commented Jul 26, 2013

http://jsfiddle.net/qa2kx/10/

Examples 1 and 2 are roughly expected behavior: functions in a scope resolution chain are treated as objects instead of executed when a property of the function is accessed, but executed when the function is the last element in the chain.

Examples 3 and 4 are unexpectedly inconsistent: I created a self() function property on the function, which returns the original function. In each case self() is executed to return the function, but when the function is a positional parameter in the helper call, it is further wrapped in a Mustache.resolve callback; when it is in the hash, it is not wrapped in this callback.

@ghost ghost assigned andykant Sep 27, 2013

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 23, 2013

Contributor

@bmomberger-reciprocity Thanks so much for all the issues you submit. To make things easier for us, can you include a brief explanation of what's breaking so that as we go through these issues, we don't have to study the fiddle to get what is breaking.

Here's my summary (let me know if I have it wrong)

Passing data like:

var MyConstructor = can.Construct.extend({text: "static text"},{})
MyConstructor.altConstructor = function(){
  return MyConstructor;
}
var data = {
  Construct: MyConstructor
}

with a helper that returns the hash's clazz's text property or the option's text property like:

can.Mustache.registerHelper("helper", function(options) {
      return options.hash ? options.hash.clazz.text : options.text;
  })

to the following templates, produces the result after the =>

{{Construct.constructor.text}} => "static text"
{{Constructor.altConstructor.text}} => ""

{{Construct.constructor}}{{text}}{{/Construct.constructor}} => ""
{{Construct.altConstructor}}{{text}}{{/Construct.altConstructor}} => ""

{{helper Construct.constructor}} => ""
{{helper Construct.altConstructor}} => ""

{{helper clazz=Construct.constructor}} => ""
{{helper clazz=Construct.altConstructor}} => "bar"

Basically, we need to decide to use the function or the function's result in the cases above. Ideally we should consider if the function:

  • is a can.Construct or a can.compute.
  • is the "end" of the chain
  • is passed to a helper

Any thoughts on what would make the most sense?

Contributor

justinbmeyer commented Oct 23, 2013

@bmomberger-reciprocity Thanks so much for all the issues you submit. To make things easier for us, can you include a brief explanation of what's breaking so that as we go through these issues, we don't have to study the fiddle to get what is breaking.

Here's my summary (let me know if I have it wrong)

Passing data like:

var MyConstructor = can.Construct.extend({text: "static text"},{})
MyConstructor.altConstructor = function(){
  return MyConstructor;
}
var data = {
  Construct: MyConstructor
}

with a helper that returns the hash's clazz's text property or the option's text property like:

can.Mustache.registerHelper("helper", function(options) {
      return options.hash ? options.hash.clazz.text : options.text;
  })

to the following templates, produces the result after the =>

{{Construct.constructor.text}} => "static text"
{{Constructor.altConstructor.text}} => ""

{{Construct.constructor}}{{text}}{{/Construct.constructor}} => ""
{{Construct.altConstructor}}{{text}}{{/Construct.altConstructor}} => ""

{{helper Construct.constructor}} => ""
{{helper Construct.altConstructor}} => ""

{{helper clazz=Construct.constructor}} => ""
{{helper clazz=Construct.altConstructor}} => "bar"

Basically, we need to decide to use the function or the function's result in the cases above. Ideally we should consider if the function:

  • is a can.Construct or a can.compute.
  • is the "end" of the chain
  • is passed to a helper

Any thoughts on what would make the most sense?

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 28, 2013

Contributor

I would expect that in all cases we would use the function and not the function's result if we are not at the "end" of the chain. This is how it works in Handlebars and Mustache proper. So using @justinbmeyer's example of

var MyConstructor = can.Construct.extend({text: "static text"},{})
MyConstructor.altConstructor = function(){
  return MyConstructor;
}
var data = {
  Construct: MyConstructor
}

I would not expect the following to work at all, and it doesn't.:

{{Construct.altConstructor.text}}

but I would assume this should work, but it doesn't:

{{#Construct.altConstructor}}{{text}}{{/Construct.altConstructor}}

While I'm not 100% sure on helpers, I would imagine it would make more sense to pass in the raw function rather than run the function through a can.compute. Thoughts on this?

Contributor

imjoshdean commented Oct 28, 2013

I would expect that in all cases we would use the function and not the function's result if we are not at the "end" of the chain. This is how it works in Handlebars and Mustache proper. So using @justinbmeyer's example of

var MyConstructor = can.Construct.extend({text: "static text"},{})
MyConstructor.altConstructor = function(){
  return MyConstructor;
}
var data = {
  Construct: MyConstructor
}

I would not expect the following to work at all, and it doesn't.:

{{Construct.altConstructor.text}}

but I would assume this should work, but it doesn't:

{{#Construct.altConstructor}}{{text}}{{/Construct.altConstructor}}

While I'm not 100% sure on helpers, I would imagine it would make more sense to pass in the raw function rather than run the function through a can.compute. Thoughts on this?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 28, 2013

Contributor

Yes, helpers should always get the raw stuff.

You are positive about:

{{Construct.altConstructor.text}}

not working in mustache and handlebars? Mustache describes what should happen when a function gets passed?

Contributor

justinbmeyer commented Oct 28, 2013

Yes, helpers should always get the raw stuff.

You are positive about:

{{Construct.altConstructor.text}}

not working in mustache and handlebars? Mustache describes what should happen when a function gets passed?

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 28, 2013

Contributor

Yup,

Here's an example of what happens in Handlebars: http://jsfiddle.net/mE49M/195/
Here's another example of what happens in Mustache: http://jsfiddle.net/rttBM/3/

You can see that, unless at the end of the "chain" neither of them actually call functions. This is evident by

{{interests.projects.length}}

What gets rendered out isn't the length of what the array that the projects function would return, but rather the number of expected arguments.

This can also be seen by

{{interests.projects.otherProp}}
{{interests.projects.otherFunc}}   

Both of which skip over calling the projects function, and look for otherProp and otherFunc as properties on projects

data.interests.projects.otherProp = "prop bar"
data.interests.projects.otherFunc = function() {
    return "func bar";
}
Contributor

imjoshdean commented Oct 28, 2013

Yup,

Here's an example of what happens in Handlebars: http://jsfiddle.net/mE49M/195/
Here's another example of what happens in Mustache: http://jsfiddle.net/rttBM/3/

You can see that, unless at the end of the "chain" neither of them actually call functions. This is evident by

{{interests.projects.length}}

What gets rendered out isn't the length of what the array that the projects function would return, but rather the number of expected arguments.

This can also be seen by

{{interests.projects.otherProp}}
{{interests.projects.otherFunc}}   

Both of which skip over calling the projects function, and look for otherProp and otherFunc as properties on projects

data.interests.projects.otherProp = "prop bar"
data.interests.projects.otherFunc = function() {
    return "func bar";
}
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Oct 28, 2013

Contributor

Your first fiddle isn't a great example because it's reasonable that mustache might look for the property on the function, if it doesn't exist, call the function. However, I tried that:

http://jsfiddle.net/mE49M/196/

And it seems like it doesn't take this approach.

But ... I'm not sure how I feel about it because can.Component uses a lot of functions for derived values. I'd rather not have people declare all of them as can.computes.

Contributor

justinbmeyer commented Oct 28, 2013

Your first fiddle isn't a great example because it's reasonable that mustache might look for the property on the function, if it doesn't exist, call the function. However, I tried that:

http://jsfiddle.net/mE49M/196/

And it seems like it doesn't take this approach.

But ... I'm not sure how I feel about it because can.Component uses a lot of functions for derived values. I'd rather not have people declare all of them as can.computes.

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 29, 2013

Contributor

Dunno, maybe it should be presented to the rest of Bitovi and get their thoughts?

Contributor

imjoshdean commented Oct 29, 2013

Dunno, maybe it should be presented to the rest of Bitovi and get their thoughts?

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Oct 29, 2013

Contributor

Functions aren't part of the spec so there isn't an official "it should be this way". I'd recommend doing something similar to how we do helpers. Check for a matching path on the function object first, then if there wasn't one execute the function and apply the path on that.

It could get messy once you start having multiple functions in the chain though.

Contributor

andykant commented Oct 29, 2013

Functions aren't part of the spec so there isn't an official "it should be this way". I'd recommend doing something similar to how we do helpers. Check for a matching path on the function object first, then if there wasn't one execute the function and apply the path on that.

It could get messy once you start having multiple functions in the chain though.

@andykant andykant removed their assignment Mar 21, 2014

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Apr 28, 2014

Contributor

Just an update on this issue, I would lean towards:

Resolving paths with function objects

  • Initially treat the function as a plain object, checking the next lookup in the path
  • If the next lookup is found on the plain object, use it
  • Otherwise, execute the function and attempt a lookup on that object

Interpolation

  • If the final object in the path is a function, always execute it and return the result
    This should provide compatibility for the current interpolation method (where it will always execute the function and do lookups on the result, regardless of where it is in the path).

Helpers

  • All final objects, including functions, should be passed as raw objects
Contributor

andykant commented Apr 28, 2014

Just an update on this issue, I would lean towards:

Resolving paths with function objects

  • Initially treat the function as a plain object, checking the next lookup in the path
  • If the next lookup is found on the plain object, use it
  • Otherwise, execute the function and attempt a lookup on that object

Interpolation

  • If the final object in the path is a function, always execute it and return the result
    This should provide compatibility for the current interpolation method (where it will always execute the function and do lookups on the result, regardless of where it is in the path).

Helpers

  • All final objects, including functions, should be passed as raw objects

@andykant andykant self-assigned this Apr 28, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 29, 2014

Contributor

I am not so sure we should treat as plain objects and then treat as a function. It means we won't only be testing "context" objects for some path, but any function we find. Also, there will be some weirdness around properties like length which are on all functions. Say I had a compute like:

var items = can.compute(new can.List(["a","b"]))

And a template:

{{#items}}{{.}}{{/items}}

This would hit the function's length property instead of reading the compute's value.

I think the easiest thing is to treat functions that inherit from can.Construct as objects, every other function is run.

Contributor

justinbmeyer commented Apr 29, 2014

I am not so sure we should treat as plain objects and then treat as a function. It means we won't only be testing "context" objects for some path, but any function we find. Also, there will be some weirdness around properties like length which are on all functions. Say I had a compute like:

var items = can.compute(new can.List(["a","b"]))

And a template:

{{#items}}{{.}}{{/items}}

This would hit the function's length property instead of reading the compute's value.

I think the easiest thing is to treat functions that inherit from can.Construct as objects, every other function is run.

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Apr 29, 2014

Contributor

That seems like a fair compromise.

Contributor

andykant commented Apr 29, 2014

That seems like a fair compromise.

@bmomberger-reciprocity

This comment has been minimized.

Show comment
Hide comment
@bmomberger-reciprocity

bmomberger-reciprocity Apr 29, 2014

Contributor

It might be easier to go the other way and treat functions as objects first and then read their results. However, for the sake of simplicity, can.computes should not be treated as functions. The documentation should be exceedingly clear about this; i.e., "A can.compute will always be Scope-read as its resolved value, never treated as a function object."

In this model, the length property on a function should be a special case that is never used.

Additionally, if I want to ensure that I'm using the resolved value of a function, I'd like to be able to leverage Function.prototype.call, i.e.

var obj = { 
  foo : function() {
    return { bar : "baz" };
  }
}
obj.foo.bar = "quux";
....
{{! with obj as the context }}
{{foo.bar}} {{! -> "quux" }}
{{foo.call.bar}} {{! -> "baz" }}

Can we also be sure that they're treated the same when passed as positional parameters versus hash parameters (the original purpose of this ticket)? Thanks. :)

Contributor

bmomberger-reciprocity commented Apr 29, 2014

It might be easier to go the other way and treat functions as objects first and then read their results. However, for the sake of simplicity, can.computes should not be treated as functions. The documentation should be exceedingly clear about this; i.e., "A can.compute will always be Scope-read as its resolved value, never treated as a function object."

In this model, the length property on a function should be a special case that is never used.

Additionally, if I want to ensure that I'm using the resolved value of a function, I'd like to be able to leverage Function.prototype.call, i.e.

var obj = { 
  foo : function() {
    return { bar : "baz" };
  }
}
obj.foo.bar = "quux";
....
{{! with obj as the context }}
{{foo.bar}} {{! -> "quux" }}
{{foo.call.bar}} {{! -> "baz" }}

Can we also be sure that they're treated the same when passed as positional parameters versus hash parameters (the original purpose of this ticket)? Thanks. :)

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 30, 2014

Contributor

Not treating them as functions would break a lot of code, examples, etc. For example:

Person = can.Map.extend({
  fullName: function(){
    return this.attr("first")+this.attr("last");
  }
})
var me = new Person({first: "Justin", last:"Meyer"})

template({me: me});
{{me.fullName}}
Contributor

justinbmeyer commented Apr 30, 2014

Not treating them as functions would break a lot of code, examples, etc. For example:

Person = can.Map.extend({
  fullName: function(){
    return this.attr("first")+this.attr("last");
  }
})
var me = new Person({first: "Justin", last:"Meyer"})

template({me: me});
{{me.fullName}}
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Apr 30, 2014

Contributor

@bmomberger-reciprocity I will definitely be fixing the consistency of the parameters.

Contributor

andykant commented Apr 30, 2014

@bmomberger-reciprocity I will definitely be fixing the consistency of the parameters.

andykant added a commit that referenced this issue Apr 30, 2014

Fixed can.Construct-derived static properties being inaccessible in M…
…ustache #450

Also fixed consistency issues in handling helper arguments
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Apr 30, 2014

Contributor

I added the following fixes:

  • can.Construct functions are never executed in lookups
  • Anonymous functions that are not the final lookup are always executed
  • Helpers arguments/hash objects are now consistent
    • Functions that are the "final" object are always passed as the original object and never executed
    • The argument/positional parameter way things were handled were the proper way

For instance, the helper in your fiddle would need to be changed to:

can.Mustache.registerHelper("cat", function(options) {
    var clazz = options.hash ? options.hash.clazz : options;
    // When using the anonymous function containing foostructor, it will need to be executed
    return clazz.text || clazz().text;
});
Contributor

andykant commented Apr 30, 2014

I added the following fixes:

  • can.Construct functions are never executed in lookups
  • Anonymous functions that are not the final lookup are always executed
  • Helpers arguments/hash objects are now consistent
    • Functions that are the "final" object are always passed as the original object and never executed
    • The argument/positional parameter way things were handled were the proper way

For instance, the helper in your fiddle would need to be changed to:

can.Mustache.registerHelper("cat", function(options) {
    var clazz = options.hash ? options.hash.clazz : options;
    // When using the anonymous function containing foostructor, it will need to be executed
    return clazz.text || clazz().text;
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment