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

Bug/Fix accessing parentData(?) when having empty data context #4797

Conversation

Projects
None yet
3 participants
@TomFreudenberg
Copy link
Contributor

commented Jul 23, 2015

You will find a working leaderboard example for this usecase at:

http://meteorpad.com/pad/xSYbj5cpx4GxC3ExR/Leaderboard_Template_parentData_Bug_Example

This works fine until you change LINE 13 in main.html from

{{> leaderboard dummy="this must be filled otherwise there is no data context"}}

into

{{> leaderboard}}

Without the dummy argument, the data context is not initialized and therefore you can not access it via this.data nor Template.parentData(1).

So my suggestion and patch checks always for the existing of a data context or will create an empty object.

@TomFreudenberg TomFreudenberg changed the title Blaze/Spacebars: Bug accessing parentData(?) when having empty data context Blaze/Spacebars: Bug/Fix accessing parentData(?) when having empty data context Jul 23, 2015

@Slava Slava changed the title Blaze/Spacebars: Bug/Fix accessing parentData(?) when having empty data context Bug/Fix accessing parentData(?) when having empty data context Jul 28, 2015

@Slava Slava added the confirmed label Jul 28, 2015

@Slava

This comment has been minimized.

Copy link
Member

commented Jul 28, 2015

Thanks for the reproduction.

@dgreensp is it something you would be interested in looking at?

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2015

Thanks for linking to MeteorPad, that's great.

This behavior is as intended. {{> leaderboard}} does not create a data context, but passes on the current data context (if any). A better place to store ReactiveVars is on the template instance.

@dgreensp dgreensp closed this Jul 28, 2015

@TomFreudenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2015

Hi David ( @dgreensp )

The problem on storing it on template instance is, that you can't access those values from child templates via parentData.

If you have a look at the MeteorPad, there is this style stored in a parent template and each child template will get any reactive changes. We made this like having props in React.

But without the data context on parent it can't be done.

So:

this.data.mystyle = new ReactiveVar()

can be accessed be child like

Template.parentData(1).mystyle

but not when created with

this.mystyle = new ReactiveVar()

Did you recognized that?

Cheers,
Tom

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

Indeed, people want to access ReactiveVars in parent templates all the time, and it is a shame we don't have a better official pattern for that. Most people walk up the "view" hierarchy for cases like this. See, e.g.: https://forums.meteor.com/t/access-parent-template-reactivevar-from-child-template/973/8 .

@TomFreudenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

But isn't to have a data context on each Template a better way than to use additional (hacky) packages? If I read the official documentation I would expect that I may have access to that context everytime. Why do you think that this isn't a good solution?

With the patch parentData() is always working.

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

Hi Tom,

data is the Handlebars/Spacebars data context, which exposes properties from JavaScript to the template. When you use {{#with}} or {{#each}}, you enter a new data context, and when you write {{> template}} with no arguments, you don't. Data contexts are not tied to templates; they can be different for different parts of a template, and the same across calls to other templates. That's just fundamental to how the templating works, and changing it would break pretty much every Meteor app.

In your example, there are two data contexts. The outer one is established by {{> leaderboard dummy=...}}, and the inner one is established by {{#each players}}. When you write Template.parentData(1), you are stepping out of the inner data context (out of the #each). If you changed leaderboard to have two layers of loop for some reason, you would have to change the 1 to a 2, and if you added a #with or #each to the player template, you would have to bump the number to 3. If {{mystyle}} were called from multiple places, there might be no single number that works. In addition, if the dummy template argument were something that could reactively change, it would cause the data object to be replaced by a new object and you'd lose your ReactiveVar.

The template instance solution I linked to really is more elegant, because template instance objects are 1:1 with instantiations of templates, they stick around for the entire life of the template, and if you want to read a value from "up the chain" of templates, you can ask for the leaderboard template by name! That means the coupling between the player and leaderboard templates is robust even if you refactor your templates so that leaderboard no longer calls player directly, or player no longer calls mystyle directly.

Hope this helps. I'm aware that understanding the data context may be confusing.

@TomFreudenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

Hi David @dgreensp

thanks for your long reply. I got your message how templates and data contexts are working, I may miss something but I do not think that this might break pretty much every Meteor app.

The patch will just create a data context only if there is none. That means at least only the first template in a chain will be affected by this patch if there was no data context given yet.

Lets say for an existing app

template main
  {{> leaderboard}}

template leaderboard
  {{#each players
    {{> player

template player
  {{this

In this case only template leaderboard will get a data context attached but this does not change anything on relative numbering to parentData() for the later ones, or does it?

From my understanding, when there is once a data context this is used also for any given sub-templates and with the patch there would be nothing changed to this behavior. Am I wrong?

The problem you describe with relative numbering to the right context, you are correct. From my point of view this is something like already existing app design decisions for each developer.

At least I can agree that it would be best if something like the linked solution would be already available by an "official" API.

Tom

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

This PR changes more than that. It does the equivalent of replacing {{> foo}} with {{> foo emptyObject}} for all no-argument {{> foo}}. It makes a change in the template compiler, so there is no such thing as the current data context at that point. dataCode is the code generated based on the argument or arguments to the inclusion, so it is set if and only if there are arguments.

It breaks the app that runs our unit tests, for one (as well as a bunch of unit tests).

@TomFreudenberg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

Well I see ... thanks David for clarification. Should I try to rewrite the patch that it meets the above "wished" feature or is it in any case not really useful?

@dgreensp

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2015

It's not something we want to change right now. Thanks for the offer, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.