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

Make {{#with}} work again with yields and contentFor #1

Open
cmather opened this issue Mar 4, 2014 · 10 comments
Open

Make {{#with}} work again with yields and contentFor #1

cmather opened this issue Mar 4, 2014 · 10 comments

Comments

@cmather
Copy link
Owner

cmather commented Mar 4, 2014

You cannot currently use the {{#with}} block helper to set a data context for a yield region. You can only set a global data context for the layout using layout.data({}); This is obviously no bueno.

@cmather
Copy link
Owner Author

cmather commented Mar 4, 2014

The problem is that when you call the yield helper with arguments, the arguments become the new data context. And there's no way to disambiguate no arguments vs parent arguments vs arguments as far as I can tell.

Example:

{{> yield region="footer"}}

In this case, the yield component this.get() will result in {region: "footer"}.

{{> yield}}

In this case, the yield component this.get() will result in the parent's data context object.

@tmeasday
Copy link
Contributor

tmeasday commented Apr 2, 2014

I'm trying to figure this one out.

Does it not make sense for yield itself to call something like findComponentWithProp('data', self) to find the closest parent component that has data, then render the template with that data?

Then potentially layout.data() can simply do that -- set the data for the layout component.

Is there a failing test that I can try and make go green here?

@tmeasday
Copy link
Contributor

tmeasday commented Apr 2, 2014

( the first bit being similar to what {{..}} does in ui/fields.js )

@cmather
Copy link
Owner Author

cmather commented Apr 2, 2014

Yeah there's probably something we can do here. Basically just ran out of time. Maybe we could also just set the data property of the yield component to undefined. Need to check but maybe that will always take the args data context out of the picture. I have the flu and my mind is a little cloudy so this could be total nonsense :)

@tmeasday
Copy link
Contributor

tmeasday commented Apr 8, 2014

@cmather

I added a failing test for this to the 1-inherit-data branch (shall I just pull this into dev?)

I think it's basically impossible to achieve this however. Here's my logic.

AFAIK:

{{> yield region='foo'}}

is equivalent to (and indistinguishable from):

{{#with region='foo'}}{{> yield}}{{/with}}

But we'd need to somehow distinguish between that and:

{{#with dataContext}}{{> yield}}{{/with}}

The only way I could see to do that is to check if dataContext.region is set, which is never going to work. Am I missing something?

tmeasday added a commit that referenced this issue Apr 9, 2014
tmeasday added a commit that referenced this issue Apr 9, 2014
@tmeasday
Copy link
Contributor

tmeasday commented Apr 9, 2014

@cmather

I just pushed another test and a "fix" to 1-inherit-data which serves my purposes. It's very much related to the problem here.

Here's the test case: https://github.com/EventedMind/blaze-layout/blob/1-inherit-data/layout-test.html#L67-L73

Can you see the issue? That html translates to:

{{#with title="ok"}} // 1
  {#with template="LayoutWithOneYield"}} // 2
    {{#Layout}}
      inner{{title}}
    {{/Layout}}
  {{/with}}
{{/with}}

So the data context for the yields in the layout is {template: "LayoutWithOneYield"} from (2) (this is normal usage, we can't call .setData() on the Layout, so we are stuck with the default). Obviously we'd expect it to be {title: "ok"} from (1).

The solution (which is hacky) is to set the default data to be the data of the "parent's parent" (or next closest component with data).

This won't work if the Layout is called like {{#Layout}} (i.e. with no arguments), but why would you ever do that?

IDK. Perhaps we just wait on MDG's solution to this problem and workaround it by using {{#with ../../..}} in our templates for now. What do you think?

@cmather
Copy link
Owner Author

cmather commented Apr 9, 2014

Yeah, I think you identified some of the issues. My thinking is to wait a bit to see how MDG ends up changing key value args. I think it's pretty clear that we need a way to disambiguate inclusion/block arguments, which are really more like constructor arguments, from data contexts.

This gets even trickier when you think about compiled templates vs. js components. Currently, the key value arguments of a block helper actually become a new component in the parent tree (an anonymous UI.With component). So we don't ever know whether the parent data context was created because of key/value args or because there's a user defined component up there.

For example:

<template name="CompileMeBaby">
  {{#Layout template="MasterLayout"}}
    some content
  {{/Layout}}
</template>

compiles to a render function that looks something like this:

return UI.With(function () {
 return { template: 'MyasterLayout' };
}, UI.Block(function () {
  return this.__content;
}));

If key value args became a special property or an argument to our component init function or constructor, I think we'd be in business.

@tmeasday
Copy link
Contributor

tmeasday commented Apr 9, 2014

Yes. That's what I figured from inspecting the component heirarchy. Thanks for explaining how it happens behind the scenes :)

I see @avital literally just posted to core about it, so I take your word for it that they are very aware of the problem. I can live with putting in some {{#with blocks for now.

@tmeasday
Copy link
Contributor

tmeasday commented May 2, 2014

Actually this isn't true!

It compiles to something like

return Spacebars.TemplateWith(function () {
 return { template: 'MyasterLayout' };
}, UI.Block(function () {
  return this.__content;
}));

And TemplateWith is different to UI.With, in that it has a ___isTemplateWith property that we can use to distinguish them. This is how UI.InTemplateScope manages to get around this exact same problem.

We should discuss this and update BL to set a better default data context.

tmeasday added a commit that referenced this issue May 6, 2014
Conflicts:
	layout-test.html
tmeasday added a commit that referenced this issue May 6, 2014
@tmeasday tmeasday mentioned this issue May 6, 2014
@tmeasday
Copy link
Contributor

tmeasday commented May 6, 2014

Awesome. working. Please review #18 when you have a chance.

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

2 participants