GeppettoJS / backbone.geppetto

Bring your Backbone applications to life with an event-driven Command framework.
http://geppettojs.github.com/backbone.geppetto/
MIT License
203 stars 28 forks source link

parent context not working #48

Closed mmikeyy closed 10 years ago

mmikeyy commented 10 years ago

Hi, I've spent quite a bit of time playing with Geppetto and I kind of like the way it forces me to structure my code. I have a lot of cases where data is shared among a parent view, a child, a sub-child etc. Often, it becomes a nightmare to sort out where exactly the data is inherited from.

Geppetto in itself is a bit nightmarish to learn because, among other things, only simplistic examples are given. Also the documentation is incomplete. And parent contexts are mentioned as being under development, but they are used here and there as if they were available.

My main gripe with Geppetto has to do with parentContext given as a parameter to the bindContext function. When one does this, the view becomes unresponsive to dispatches.

I delved into the code to attempt to find why parentContexts do not work. For me, they are an important part of the framework as I want it be easy to access objects easily without having to remember if they are local, inherited from a parent, a grand-parent etc.

The problem seems to be with the creation of the resolver for a context. Lines 183 and following:

    if (this.options.resolver) {
        this.resolver = this.options.resolver;
    } else if (this.parentContext) {
        this.resolver = this.parentContext.resolver.createChildResolver();
    } else if (!this.resolver) {
        this.resolver = new Resolver(this);
    }

This line:

  this.resolver = this.parentContext.resolver.createChildResolver();

accesses the parent context resolver and lines 83 and following establish the link with the parent after creating the child resolver:

    createChildResolver: function() {
        var child = new Resolver(this._context);
        child.parent = this;
        return child;
    },

The problem with this apparently (I'm a Geppetto rookie...) is that the child resolver that is returned is a resolver based on the parent context, not the child.

If we look at the following line (188): this.resolver = new Resolver(this);

it is clear that the constructor expects a parameter which is the resolver's context (I know, it's obvious)

Now line 84 creates a resolver based on the parent context (parent context is this._context)

         var child = new Resolver(this._context);

Remember that we're in the context of the parent context resolver (this = parentContext.resolver). And from within this context, we are creating again the same resolver which is already the current context.

It then assigns "this" to child.parent (which makes sense)

         child.parent = this;

This "child", with the parent context resolver as a parent and built around the parent context now becomes the child context resolver.

The child context is totally left out of this process. The child context resolver is built around the parent context, and the parent context is its parent.

Actually, I don't know why one has to go modify the parent context or its resolver at all. We always want to go up the tree (from child to parent), not down. All we need to do is reference the parent in the child. The parent does not even need to know that a child even exists.

So I changed the code a little bit and my program started working. A view that was totally non-responsive to dispatches became responsive. I could wire a view with a reference to an object two hierarchical levels down without problem.

Here's what I've done to lines 180 and following:

    if (this.options.resolver) {
        this.resolver = this.options.resolver;

***** two lines commented out //} else if (this.parentContext) { //this.resolver = this.parentContext.resolver.createChildResolver();

    } else if (!this.resolver) {
        this.resolver = new Resolver(this);
    }

**** these lines added if (this.parentContext){ this.resolver.parent = this.parentContext.resolver }

Of course, the 'createChildResolver' method also becomes useless.

That's it. It seems to be too simple. But it works. Am I missing something? Will this explode in my face tomorrow?

creynders commented 10 years ago

Hi @mmikeyy if I remember correctly this is stuff I (partially?) wrote. I'll have a look but probably need some time to grok this. Hope to get back to you with more on this soon.

creynders commented 10 years ago

@mmikeyy I finally found some time to take a look. And I think you're right. We need to test this thoroughly though.

ping @geekdave

mmikeyy commented 10 years ago

Good! I'm using Geppetto with a big program with lots of views and sub-views sharing data. Parent contexts do save the day. I'be been using my patched Geppetto for a good while now and it's been working flawlessly .

While we're at it let me mention another fix if I may.

Geppetto automatically adds a close method to any view that doesn't have it and also adds a listener for "close" events that will automatically destroy the bound context. (Lines 217...) Now neither Backbone nor Marionette use close to close a view. Marionette recently switched to destroy. Unless I'm missing something Geppetto should switch to destroy as well (as my patched version already does).

That's it for now. (More to come!)

Le 26 août 2014 03:22, "creynders" notifications@github.com a écrit :

@mmikeyy I finally found some time to take a look. And I think you're right. We need to test this thoroughly though.

ping @geekdave

— Reply to this email directly or view it on GitHub.

geekdave commented 10 years ago

PR for this: https://github.com/ModelN/backbone.geppetto/pull/54

geekdave commented 10 years ago

Closing as duplicate of #54