partial parameters are lower on the stack than context

[carchrae]

When giving a partial parameters, they are pushed down the stack and the previous context is pushed onto head. If there are two variables with the same name, then the partial parameter will be hidden by the higher context.

This is confusing default behaviour and seems inconsistent to how sections operate.

I'm aware of the changes introduced by #132 - but this seems like a hack to fix the issue of not searching a higher context ( #271 ).

See example below. Expect x=2, but it is x=1.

dust.loadSource(dust.compile("partial says x={x}", "part"));
dust.loadSource(dust.compile("{>part x=\"2\" /}", "test"));
var data = {
  x :1
};

dust.render("test", data, function(err, out) {
  $("body").html(out);
});

[rragan]

I didn't pay close enough attention to this when @carchrae first raised the point. Historically we got to this point as follows:

• Original dust had no params on partials
• params were added when LinkedIn picked up dust (+1 for params on partials)
• People using params on partials discovered that a path reference that worked in the context where the partial was invoked would suddenly fail if they added any params to the partial invocation. This was because paths would not look beyond current context back then and the params created an extra context which blocked the main context from the point of invocation. The Partial with parameters was not getting the correct context #132 change resolved this by swapping the initial context with the params so the initial context remained the immediate context and paths worked fine again
• We finally changed path references to not be constrained to the immediate context. This turns out to be an alternative fix to the problem arising in Partial with parameters was not getting the correct context #132.

Developers wanting to rely on libraries of standard partials are seeing random breakages based on what happens to be in the context at the point of invocation. For example, consider a partial to generate HTML input tags in a standard way that accepts name as a param.

A use of this partial might be:

{>textInput name="amount"/}

This works fine and generates as long as there is no value for "name" in the immediate context. If, however, context is:

{"name": "other"}

then the partial's output will be and the partial's specification is broken.

This change would NOT be backward compatible but the current situation seems a serious problem also.

[jimmyhchan]

FWIW original Dust only supported params in sections and it had the odd behavior of preferring the current context over the params context. Keeping this behavior was purely for consistency. #358 is a PR for adding params to inline partials and we are seeing this issue yet again.

Dumping back compat and addressing this consistently everywhere is not off the table.

[carchrae]

I think the issue here is that the preferred context order often changes depending on use, so either default is always going to confuse/annoy someone.

Perhaps forcing section name scoping {#sec param : 1} {sec.param} {/sec} or the ability to manipulate the context stack, eg, {#sec context = stack(head,context) } (or something along those lines)

[rragan]

I think the ordering issue is largely taken care of by the changes in 2.0.0 to allow path references to search further up the stack. Original dust rules for section parameter names taking precedence over context data fits my mental model of what a reference inside the partial should access (e.g. the parameter value). The documentation even talks about aliasing parameter names to leave access to context values accessible.

Being consistent with section scoping rules appeals as there is only one set of rules to learn. We just need to undo the swapping that was done earlier. Since I like to think of partials as encapsulated modular things, the idea of having them reach into the outer context of the caller does not appeal to me -- I'd rather they have data passed in explicitly. However, I recognize that some people use them like macros to avoid repeating code and expect to get data from the current context. I see where we need to add complexity with a choice to control the stacking order.

[carchrae]

+1 for consistent scoping. that was the main issue i had.

i don't know if it is implemented, but i would love to see name clash
warnings in the console. i have manually implemented that in my own code
whenever i push things on the context.

On Fri, Nov 22, 2013 at 11:13 AM, Richard Ragan notifications@github.comwrote:

I think the ordering issue is largely taken care of by the changes in
2.0.0 to allow path references to search further up the stack. Original
dust rules for section parameter names taking precedence over context data
fits my mental model of what a reference inside the partial should access
(e.g. the parameter value). The documentation even talks about aliasing
parameter names to leave access to context values accessible.

Being consistent with section scoping rules appeals as there is only one
set of rules to learn. We just need to undo the swapping that was done
earlier. Since I like to think of partials as encapsulated modular things,
the idea of having them reach into the outer context of the caller does not
appeal to me -- I'd rather they have data passed in explicitly. However, I
recognize that some people use them like macros to avoid repeating code and
expect to get data from the current context. I see where we need to add
complexity with a choice to control the stacking order.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/313#issuecomment-29100489
.

[rragan]

If not currently there, name clash warnings should be part of the dust debug work in 2.2.0