ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

Did components lose their own context stack? #428

Closed skeptic35 closed 10 years ago

skeptic35 commented 10 years ago

From the way some of my components suddenly behave it would apper so. Check out this simple JSFiddle using Ractive 0.4.0: http://jsfiddle.net/y4unF And the same same setup with 0.3.9: http://jsfiddle.net/y4unF/1/

Seems like the component all of a sudden has access to the data property of it's parent! If that change of behaviour was intentional, then I'd seriously beg you to reconsider, as it is imho absolutely not how components should behave. The way I understood it a component was supposed to be kind of a polyfill that could be used in a variety of projects. But for that they do have to behave in a predictable way, meaning their behaviour should solely depend on the data that ist passed on to the component, not on some data that may or may not be present in the component's parent.

codler commented 10 years ago

I hope this is a bug and not a feature

Rich-Harris commented 10 years ago

It's a feature! Apologies for the lack of signposting, I see how this could throw you. But bear with me:

I would often find that I was writing things like this:

<widget foo='{{foo}}' bar='{{bar}}'/>

(Well not literally but you get the idea.) In other words I want child components, possibly nested several levels deep, to be able to access ancestor scopes. The example above might not be especially convincing, but there are lots of situations where having to explicitly pass data down from one level to the next is extremely cumbersome. For example if you're building a chart, you might have x1, x2, y1, y2, xScale, yScale and so on - individual bits of the chart (axes, ticks, annotations, the data itself) need to know what they are. Suddenly your templates look very messy.

What I found in practice was that when it gets that messy, I don't bother creating components any more - I just dump everything inline.

But there's already a perfect metaphor that lets us have our cake and eat it: JavaScript closures, one of the best features of the language:

// THIS...
var functionOne, functionTwo;

functionOne = function () {
  var foo = 1, bar = 2;

  functionTwo = function () {
    var bar = 3, baz = 4;
    // we can access outer foo, but not bar
    console.log( foo, bar, baz ); // 1, 3, 4
  };
};
<!-- ...IS ANALOGOUS TO THIS: -->
<widget-one foo='1' bar='2'/>

<!-- widget-one template -->
<widget-two bar='3' baz='4'/>

<!-- widget-two template -->
{{foo}} {{bar}} {{baz}}

<!-- renders as -->
1 3 4

It's a more flexible approach - the inner widget can be completely isolated (you can specify foo='whatever' when you invoke it, or its default data property could specify a default value for foo, even if that value is null), but it doesn't have to be.

Personally I've found my productivity increase since making this change, because I'm creating more components and putting less junk inline. It basically means that components can (in the extreme case) behave just like partials, except with custom CSS and behaviour (I'm also using the single file component definition, alongside a RequireJS loader plugin that I'll release soon.)

But I appreciate that if you're creating components for reuse between projects (as opposed to using them simply to make a particular app better structured), some of this might not be helpful. I've mentioned that you can achieve encapsulation by having default data properties (the equivalent of var foo;) - at least, if you can't, that is a bug - but maybe it should be an option:

var Widget = ({
  template: '<p>I am a widget and my foo is {{foo}}</p>',
  isolated: true // <- will not lookup `foo` on outer scopes
});

How does that sound?

browniefed commented 10 years ago

@Rich-Harris I like the isolated flag, and it's still possible to get access to the parent scope via _super () correct? Obviously accessing _parent which is technically private works but is not suggested.

I've noticed that with a lot of my heavy loop dependent stuff that is render various random cell templates if I attempt to bind data to the component it was significantly slower. So I am binding row indexes and column index then accessing the data when events fire. So having isolated true is great but it'd also be great to have a mixture, some way that when things happen that we get access to the original data context passed into the component but also the parent data?

Closures are great but not always going to work if I'm listening to an event coming out of a component, and then I have no way to access the data unless I create some sort of getter on the component that has access to that closure scope.

codler commented 10 years ago

Hm, I see the benefits, but I also see a greater chance for programming mistakes where you accidently pass unintended variables to the component. Maybe it is better if you could scope and only pass innermost context to the component. I continued with @skeptic35 example and added a loop around. That is what I mean with innermost context http://jsfiddle.net/XDT6U/

The isolate flag would be good to have.

martypdx commented 10 years ago

Like @Rich-Harris, I found myself writing pass-thru component data attributes:

{{#item}}
<widget title='{{title}}' content='{{content}}'/>
{{/item}}

I could see isolated: true being of value, particularly as a performance enhancement, if that is a valid concern.

Another option would be to be able to set the top-level data object (which is really why we are repeating the data):

<widget .='{{item}}' />

You would likely need to add supplemental data as well:

<widget .='{{item}}' title='{{other}}' />

But, I'm fine with current 0.4.0 status quo.

skeptic35 commented 10 years ago

@Rich-Harris I'm still not quite convinced that this is the way components should behave by default. But, hey, it's your library - you make the calls ;) Anyway, having default data properties actually solves the problem (http://jsfiddle.net/y4unF/2/), so no bug there. But it's a rather awkward solution as it means you'd have to default each and every parameter that you use in your template, meaning if you add a parameter in the template you also have to add a default in the component definition. From my experience things like that have a strong tendency to get forgotten. So, yes, having at least that isolated flag would be convenient (imho isolated: true should be the default, but lets not split hairs...). As for this here:

<widget foo='{{foo}}' bar='{{bar}}'/>

Whenever I find myself having to pass to many parameters, I simply wrap them. So instead of passing foo='{{foo}}' bar='{{bar}}' I'd pass an object (e.g. model='{{model}}') that has foo and bar as properties (that's actually why setting default data isn't that big a problem in my setup - but still...). That brings me directly to @martypdx suggestion of being able to pass the top level data object as a whole to the component: I distinctly remember that, when I started to use components, I thought that that was actualy the way it worked. I thought that every attribute in the component tag would become an option for the component, whereas in reality they become properties of the component's data. And I still find it strange that I can't pass anything else then data on to a component. Heretical question: Isn't that actually the real problem here? Anyways, yes, I see where this feature is coming from. But, no, I don't think that automatic inheritance of the parent's data is the way to solve it. Leaves way too much room for unwanted behaviour. But I could live with having to set isolated: true.

P.S. @browniefed: As far as I understood _super() and _parent are two completely different things. _super() is used to call a method of a ractive class you extended from and has nothing to do with components. _parent on the other hand is only present in components and is imho the only way to actually access the parent.

P.P.S @Rich-Harris Dug up this stackoverflow question I had posted, concerning components: http://stackoverflow.com/questions/20248020 Interesting quote from you there:

I don't think it's good practice for a component to be able to muck about with its parent (makes it less reusable etc)

;)

Rich-Harris commented 10 years ago

I've added in the isolated flag.

@skeptic35 heh! Good memory. I'm going to use the Ralph Waldo Emerson defence:

A foolish consistency is the hobgoblin of little minds, adored by little statesmen and philosophers and divines

It's a great quote, it gets you out of all sorts of trouble in debates, particularly after a beer or two. In other words I'd rather contradict myself than maintain a stance I've come to disagree with. In this case though I don't think it is contradictory - that quote was regarding the ability of components to use references to their ancestors, as opposed to ancestor scopes (model/viewmodel/data/whatever you want to call it). A malicious (or just badly-written) component could easily do this._parent.messThingsUpForEveryone(), which is what I was concerned about there with this._parent (though there are legitimate uses I think). But it turned out to be necessary in any case, and I didn't see any point hiding that implementation detail.

By the way, there's a nice easy way to make isolated: true the default, at least within an app:

Ractive.defaults.isolated = true;

Interesting question about attributes-as-options. I hadn't thought about that. I think it's possibly a bit problematic - it would mean that a component's model couldn't have certain properties (e.g. magic), which is one of those 'so what?' things until it causes hear-tearing bugs. Unless you were to do something like

<widget magic='true' data='{foo: {{foo}}, bar: {{bar}}, magic: "yes"}'/>

which is pretty horrific, never mind it being an aggressively breaking change. But in any case, looking down the list of initialisation options (el, template, complete, preserveWhitespace, append, twoway, modifyArrays, lazy, debug, noIntro, transitionsEnabled, magic, adapt, sanitize, stripComments, isolated, delimiters, tripleDelimiters - that's leaving out the registries like transitions and decorators), it's either impossible or doesn't really make sense to specify the option within the template in almost all cases (possible exceptions: twoway, lazy, isolated).

@browniefed - @skeptic35 is correct about _super() vs _parent. I definitely wouldn't want to discourage you from using this._parent - if that's the most efficient way to do a particular thing then I'd say that trumps concerns over the hypothetical dangers of a component running amok.

I did a bit of work this week removing unnecessary operations that take place when bindings are created between child and parent components, which might improve performance for your situation.

You mentioned listening to component events. I think that how events propagate (or 'bubble', I suppose) is one of the major weaknesses of components at the moment. Lots of room for improvement. Would be interested in any specific examples you have where events are causing headaches with context etc.

browniefed commented 10 years ago

@Rich-Harris I've done some testing of stuff out in .40 and noticed a lot of the issues I was running into with just basic data binding and observations have been fixed. .39 seemed to have issues and I noticed after investigating that the code from .39 to .40 had been almost completely refactored (at least in the component area). So props to you and appreciate the hard work you've been putting in. Been hearing a lot of great things from everyone at our company(devs/non devs alike) which is only possible because of Ractive. We are slowly but surely replacing all the Ext code that's been written over the past 6 years!

Here is just a quick example. Observers work and also upon updating data it'll actually propagate down to the component .39 http://jsfiddle.net/xRU7w/1/ - not working .40 http://jsfiddle.net/xRU7w/ - working

Rich-Harris commented 10 years ago

@browniefed Really appreciate you saying so - it really is a labour of love, and comments like yours (not to mention issues and feature ideas) are a big help.

Will close this issue as the isolated flag seems to address most people's concerns. Thanks all.