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
Implement #each .. in .. #3560
Implement #each .. in .. #3560
Conversation
Slava
commented
Jan 26, 2015
@@ -68,7 +68,7 @@ Blaze.Unless = function (conditionFunc, contentFunc, elseFunc) { | |||
/** | |||
* @summary Constructs a View that renders `contentFunc` for each item in a sequence. | |||
* @locus Client | |||
* @param {Function} argFunc A function to reactively re-run. The function may return a Cursor, an array, null, or undefined. | |||
* @param {Function} argFunc A function to reactively re-run. The function must return an object with two fields: '_variable' and '_sequence'. '_variable' is an optional alias for an item available from the Each body, '_sequence' may be a Cursor, an array, null, or undefined. For backwards-compitablity, can return just sequence (Cursor, array) not wrapped into an object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be more clear if it said something like "just a cursor or an array."
Seems legit, needs some tests though! |
Since talking to Greenspan, I was convinced that we can't afford to clone the data context because people might be putting complex objects in there. We might need to do the thing you were saying about adding some scope to the lookup chain. But then we still have the question of what Template.currentData returns.. |
It is a shallow clone. How bad is it?
|
The problem is that the binding of the methods on a complex object might be wrong, and maybe people are modifying the "this" inside event handlers or something |
The data context can have a prototype, and a lot of apps (e.g. Respondly) use some kind of Model or Component object as the data context, which can't be safely cloned. Some users also mutate the data context object directly, though this is less of an encouraged thing. I'd love to help figure out how to expose names properly with lexical scope. It would open the door to #let and be useful for @Index as well. |
Yeah, initially we had a discussion that a proper lexical scoping is required but later we opted-out to a lazier and easier to implement solution (duplication of data context on multiple levels). |
I updated the PR. Implemented @dgreensp can you please do another round of review for me? |
Wow! I will get to it as soon as I can -- early next week. Thanks for implementing this. |
What if those objects would be required to implement EJSON clone method? |
I think it would be unfortunate if some common patterns stopped being viable because we wanted to implement |
Hm, so I implemented few useful methods here. From my experience changing data context was only done because there was no easy access to template instance. Now (at least in our code) we put all custom stuff into a template instance, having also nice getters to get them across the ancestry.
What do you mean by this? |
Basically, the data context doesn't give that much flexibility, and requires you to think about the invisible omnipresent variable. Basically, "each in" is the first step towards relying less on replacing the data context all the time, and instead adding additional variables that can be referenced by name. Example 1:
Example 2:
I think the second one has a lot of benefits - in the first example, you have to remember that there's an invisible variable that you can't see, and remember where it comes from and what keys it has. In the second one, it's more visible that you're accessing the There's a reason people don't use the JavaScript with keyword very often, and in fact MDN has a big red warning about it. It's because it often causes weird problems because of invisible overwriting of the scope. What do you think? Do you agree? |
I think it is just a matter of style. In fact, it is just a matter of style. And I doubt that this is a good thing after 1.0 release. You are moving away from Handlebars. You are introducing more concepts for the same thing. You will now have to support both the new and old style. You will introduce discussions of what is better and many ways to do the same thing. At the end of the day it is really a question where you associate the context with. Do you want to name it, or you make it more like a closure. Maybe I am coming too much from the functional programming background. But having closures is something pretty natural. And this data context for me feels like a closure. Also "with" is a pretty common pattern in functional programming languages. It is called "let ... in" there. So saying that this is bed practice when this is a basic programming construct of any functional programming language is a bit stretch for me. It is true that before there was no way to navigate parent data context, but now this is possible, so one can really already use "with" as a way to overlay new variables on top of old ones. You just traverse the stack until you get a name. Similar to how I am doing with template instances. I really do not see the benefit for it. For me this is more a syntactic sugar if anything. Just a way to name stuff temporarily. But at the end of the day, this concept of a simple data context of a template is a very powerful one. We do not want to create whole programming language inside a templating engine, no? What I would rather see is simply make data context's immutable (declared them as that you should not be changing them yourself), if people want to store stuff they should be storing stuff inside a template instance. And then you can provide some getter which allows you to access a data context field in a way that only registers the dependency on that field. This is what you really want at the end. Not the special case for "each in" loop. Because when you are having complicated for each loops (which take some time to render) you anyway externalize internal content into a template:
Because Any data contexts are objects anyway. Knowing the name of the object is one, but at the end you have anyway to remember which properties it has. So I do not see much benefit in providing a way to name a property. If you really want to do that, you simply create a template helper now. |
@mitar, I have to disagree. Maybe I am coming too much from an object-oriented background, but ever since I began with Meteor, I feel like I am constantly fighting with contexts. I am not against the concept of context, but it has to have a defined meaning (like 'this' in oo-programming). In Blaze, context looks more like a vaporous global variable, with the additional oddity that it can be changed arbitrarily along the line by some block helpers you add in another file. So a few weeks ago, after a year with Meteor, I came to the conclusion that Blaze context is evil, and I decided to ban its usage: {{> projNavbar project=project contributor=contributor profile=profile}}
{{#each profiles}}
{{> profileWithPicture profile=this}}
{{/each}} {{#each profiles}}
{{#with profile=this}}
...
{{/with}}
{{/each}} What @stubailo said sounds reliving to me. I would even encourage MDG to take more radical actions. For example, I would love to have a 'no context' option, which would prevent usage of 'this' and would reset template context even when no arguments are passed. |
@mitar and @steph643, I appreciate your responses! For some more data on the matter, See the "More Consistent Handlebars Scope" section of emberjs/rfcs#15. (For present purposes we're going to ignore the further proposals they make about @mitar, it's interesting that you bring up "let" and closures, because by implementing lexical scope for each-in, it's now actually possible to implement "let"! I don't really see an analogy between the behavior of data contexts and let. When you enter an |
You can always traverse the data context parents. Now that you allow accessing that. Anyway, I think this is just a matter of style so I do not mind anyway. I got now more scared with this proposal because it looks like it is focusing on wrong things if we want to make templates extendable. (I wrote my questions at the end.) |
@mitar I don't see the comparison between
In my plan, I had an idea to introduce explicit arguments to templates like this: <template name="myTemplate" args="name age">
<div>Hello {{name}}! Today you are {{age}} years old!</div>
</template>
...
{{> myTemplate name=person.name age=calculateAge}} (The exact syntax can be different). Here Templates accept named argument explicitly, this might make it easier to use scoping rules more and rely less on the dynamic data context. |
I think this is just a question of style. When I implemented my own Scheme interpreter, I used simply a stack of environments. The only current difference is that if a variable is missing at the current level, we do not automatically traverse previous scope levels. But this is really simple to implement as a template helper now that we have So I really think that you are solving a no-problem here. If you would like to address something, thing about something really new. How to really make templates reusable and composable. Now you are just moving between very similar paradigms, without really any qualitative jump forward. Please look at Django. They had the same templating language for 10 years. And they have a great way to make templates modular and extendable. This is how you make a stable community. Do not change templates just because one style is 0.5% better than other in some opinion. Do you really want that all StackOverflow posts and all examples which already exist there would be obsolete? What I would like to see is way to extend templates. That I could say, OK, this is base template, now make the same one, just add before it this, and after it that. Or that I could parse a template through a filter. Genshi is so amazing templating engine. The most powerful I have ever seen. Even more than Django. That is a qualitative jump forward. It is not the easiest to use (XSLT is really not an easy way to develop things), but something like that would be powerful to have. |
@mitar right now we are not talking about Sashko's brainstorming hackpad. We are just talking about this new feature that still allows you to use the data context (but creates a more flexible way to achieve everything data context could with semantics closer to mainstream PL and JS itself). So let's not discuss Django keeping the same system for 10 years in this thread. Extending templates is also not covered in this PR and this is a feature that has nothing to do with this PR.
Exactly. Isn't it what is called "dynamic scoping", something that is not used as a main way to bind variables in any modern PL except for ELISP? There is also a big difference in the data context: to access a variable from a part of the "stack" that is not on the top, you need to specify a path relative to your current position on stack. So if you want to access a variable from a parent you would access it as |
I mentioned that as an example where changing things would really add some benefit. Here, for me it looks just a question of style which libraries could address them as well.
No you don't. You can use a simple helper to do that. Templates already provide things needed to implement it. Of course you could define a default helper or default traversal to parent data contexts, how I described above, but that traversal is orthogonal to changes in this pull request. In this pull request you are trying to force naming of scopes. But no language I am aware of has naming of scopes. Yes, currently with vanilla Meteor you have to define static number of levels to find both template instance or correct data context. But we addressed this with simple API additions in meteor-template-extension. If you have a template instance, you can do
Not in our code using those template extensions which did not require any changes to templates themselves. Question of changing template syntax is in my opinion just a question of changing style and I argue that changing the style is less important than having stable templating languages people can rely upon. This is why I referenced Django and its stability.
Not in our code using those template extensions.
This can be done with a simple helper. What cannot be done is extending templates. We have to do ugly internal stuff to make other methods in the template extensions package work. I think what I am saying is that in my opinion you are spending time on a wrong thing. In meantime community has found solutions and are proposing those solutions. Maybe an user study would be useful before we just theorize which one of those approaches is better for users. Yours:
Which problem are you solving? Is it a real problem or just perceived problem? Will it really allow users to do more? Or would some other solutions improve it as well, or even better? In my opinion, yea, add |
Minor clarification, Slava: Scheme's environment chain is the chain of lexical scopes, not dynamic. This PR includes each-in and The hallmark of lexical scoping is that when you write There is definitely a valid point about how if you think of the data context as a scope, you might come to different conclusions than if you think about it as an object whose properties are dumped out into the scope. However, the fact that the data context is often set to an object that comes out of Mongo, for example by There do exist Handlebars variants where lookups search all the enclosing data contexts. I think they use one at AirBnB in Rendr, if I remember correctly. It works if you are keeping in mind all the fields of all the model objects on the data context stack, and you are mindful of collisions. It hurts composability quite a lot, though. @mitar, we have template reusability and composability in mind at all times. We are working towards a world where templates take arguments, and data doesn't leak into things where it doesn't belong while at other times being strangely inaccessible. You are asking for a focus on increased composability, and almost in the same breath asking that names always filter down from parent components. You say our focus is in the wrong place if we are thinking about improving scoping, but then admit you find Meteor's built-in scoping unusable and have overridden it? What does "forcing naming of scopes" mean to you, and what are you worried about losing (not from this PR but presumably in the future)? |
It is normal that scopes have functions defined in them. So object methods become such functions. Object fields become variables in the scope.
You have a valid point here. I see the benefits of
Yes, that why in template extensions we are not providing a way to traverse data contexts to find a field, but instead search for a data context which is an instance of some JavaScript class. We use collection's
I understood @Slava above, that with
I worry that by offering to many ways to do the same thing we will introduce confusion. That instead of fixing core issues we are doing band-aids. And that in the future this will lead to deprecating of features like normal As I said, I like things proposed in this pull request in a way. I like sweet/sugary things. :-) But from things beside the style/syntax I got a feeling that we are also going into one other direction. Where using whole data contexts will be discouraged (or even deprecated). And my worry was that this is maybe just because we do not yet have nice tools to work with data contexts in place and that why they feel cumbersome. So I wanted to present some other alternatives which can make work with data contexts quite nice, declarative, decomposable. That it is maybe just a question on how you look at what a data context is. Anyway, I expressed my concerns. I think you took them into consideration and this is all what I hoped for. Maybe I do not agree with all details here, but I do not want to take more of your time anyway. |
We are very interested in user feedback. Thank you for engaging us! I think I understand now that you are mainly talking about a proposal or style where the data context is inherited, and wanting to know if that's the world we are designing for or not? We've discussed ideas before where you might be able to write something like
Or:
Where as the proposal that searches all the data contexts looks like this, and you can't tell where each field comes from:
Maybe that's not what you meant at all -- you clarified that your helpers look at the types of things -- but it sounds like you are having to define a lot of helpers right now to achieve what you want. My point about how objects that come out of the database, or model objects with methods, are sort of different from scopes, was mainly about how names get into scope. Normally names get into a scope by appearing in the source code. A model object with methods gains a name when someone adds a new instance method -- which is usually not a backwards-compatibility concern, but would be in this case! A database object gains a field when someone performs a database operation. Again, not usually how a name gets into a scope. However, there is one very important point here: Currently, since there is no real guidance on how to use the data context and how to use the template instance, we probably have a situation where people are using them in different ways. For example, some users probably use the data context as the Model, and others the Component, and others the scope. Likewise with template instances. We should definitely keep that in mind as we figure out how best to serve our users. We will almost certainly have to standardize on something, though. As you said, too many ways to do things leads to confusion. If you've picked up that we're kind of down on the data context, that's true -- a lot of what it does would be done better by real local variables (statically declared), template arguments, and better access to the template instance. However, we will keep in mind what you've said. |
I agree with your examples above. The double
Because
I found this as a good thing. :-) I like the idea of helpers and object methods being merged as one, and then inheritance in searching for the first defined.
I completely agree. I think the initial confusion was because there was no way to access template instance inside template helpers. So one had to misuse data contexts. Now, the issue is that there is no way to access parent template instances by default. But once I introduced parent method on template instances, things really became very intuitive for us. You use template instances as scopes. You add stuff you want, you use get to traverse scopes if you do not want to hard-code the number of levels (so you can refactor your templates and template inclusions without issues). You use the same code both in template helpers,
My main issue with data context is that it declares Tracker dependency on the whole document. So if II am passing DB document to the template, and I am using only one field in the template, I have to limit the DB query to only that one field if I want to optimize stuff. And if I add another field in the template, I have to update the code. I think this is the main concern separation issue I have with data contexts (and lack of getters). |
I rebased everything on top of the latest devel. Should be easier to merge later ;) |
nice! Any idea in what release this will be released? |
} | ||
return null; | ||
|
||
// 5. throw an error when called: nothing is found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't all the "not found" case, it's the "data context" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix this as part of #4036 ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I know I said I would merge that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. :-)
This will break apps that do |
Ok, I fixed bugs on devel. Consider this reviewed. |
Thank you! On Tue, May 12, 2015 at 3:46 PM, David Greenspan notifications@github.com
|
We also had |
You can add parentheses around (jobs this) to fix, right? Go But yes, this may be a problem. Proposal: if we don't see "in", do the old On Tuesday, May 12, 2015, Avital Oliver notifications@github.com wrote:
|
Ah, right! |
I guess we should have an error message that suggests this. Or just On Tuesday, May 12, 2015, Avital Oliver notifications@github.com wrote:
|
Yeah, I think this is an unnecessary breaking change, and is more of an oversight than something intentional. |
Ok, fixed in e823459 |
Thanks @dgreensp! |
Thanks guys, the {{@Index}} is going to be really useful, glad to see it's close to being implemented. |
Hey guys, were you guys aware that Handlebars had implemented something like this here? |
@rclai for some reason I cannot find the doc right now, but we looked at every syntax used in different templating languages and we noticed that Ember pushed Handlebars into implementing 2 different syntaxes. One, just the same as described in this thread, and the newer one, like described on your link. After the internal conversation, nobody liked the direction of the pipes syntax, it felt like it was trying too hard to be like ruby. |