Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SC.Binding.from only takes a path as parameter, ignoring root/propertyPath. #217

Closed
juan77 opened this Issue Nov 23, 2011 · 12 comments

Comments

Projects
None yet
6 participants

juan77 commented Nov 23, 2011

In SC 1.x we can bind by passing an object and a path to bind, for example:

SC.Binding.from(App.myController, ‘myProperty’),

And also in an object’s bind we can also pass root/propertyPath as second parameters (the from’s parameter exaclty) like for example:

MyController.bind(‘myProperty’, [myOtherController, ‘fromProperty’]);

These big ideas wasn’t found or implemented in SC 2.x, and I think that are a bug or critical, because these ideas make the magic shines.

Right now on SC2.0 we only can do using a full path like:

SC.Binding.from(‘App.myController.myProperty’),

also in object’s bind:

MyController.bind(‘myProperty’, 'App.myOtherController.fromProperty’);

And looking inside SC.Binding, we see that in the case of from method, is there a this._object attribute that is never used. check it:

https://github.com/sproutcore/sproutcore20/blob/master/packages/sproutcore-metal/lib/binding.js#L366

And looking more deeply is seems that SC.addObserver’s second parameters (that is used in connect method of SC.Binding), only takes a path, and not a tuple of root/propertyPath.

https://github.com/sproutcore/sproutcore20/blob/master/packages/sproutcore-metal/lib/observer.js#L139

For me, this is a stop-breaking in my migration from SC1.x to SC2.x because is a low level implementation and is there no other way to do it. Hope I missing something new or will be in future release, I will be the first to test it.

Thanks,
juan.-

Owner

wagenet commented Nov 26, 2011

While I wasn't involved in writing this code, I have given it some examination. It seems like the object parameter for SC.Binding.from is a relic of old code, since it's use isn't reflected in the inline documentation. It should probably be removed since, as you have noted, it is unused.

Can I ask for some detail on what you're using this for? I've never really had a need for this functionality in my extensive development of SproutCore applications. I suspect in most cases some minor changes in setup should allow you to achieve the same functionality.

Owner

wagenet commented Nov 27, 2011

@juan77: I talked with @wycats some about this and he says that we should add this feature. For now I'll remove the non-functioning code and hopefully we can add it soon. However, I still am interested in learning a bit more about what you're trying to achieve.

@wagenet wagenet added a commit that referenced this issue Nov 27, 2011

@wagenet wagenet Removed unused code. Refs #217 0a722f0
Member

ghempton commented Jan 11, 2012

+1. This is a huge feature. I am trying to avoid global bindings all together to reduce coupling. Being only allowed to bind from the current context is way too restrictive and is a pain to work with in this scenario.

Owner

krisselden commented Jan 11, 2012

That path is watched and then cleaned up when the connected object is destroyed. The object provides a context of who is listening and maintaining the binding, this could be done, probably using a guid in place of a property or global for the root in the chains so they could be cleaned up when the connected object is destroyed.

Would be nice to have a use case. You can bind to a prop of object a to obj b, just set obj b as a property of obj a and bind a chain through that property. Hiding this dependency by not making it a property doesn't mean they are decoupled. It still avoids the use of a global.

Your example:

MyController.bind('myProperty', [myOtherController, 'fromProperty']);

Using bindings as they are without global path:

MyController.set('myOtherController', myOtherController);
Ember.bind(MyController, 'myProperty', 'myOtherController.fromProperty')

juan77 commented Jan 12, 2012

Hi kselden, I appreciate your solution, and is way simple, isn’t what would like, but if you tell me that we have a performance benefit by not adding a global, I prefer to stick with your solution, if not the case I would like to gain clarity. thanks.

Owner

wycats commented Jan 21, 2012

I'm closing this ticket because it's a request to alter the overall architecture of Ember's binding system, and relatively simple workarounds exist without taking that drastic step. If you find cases where simple workarounds are not sufficient, please reopen this ticket.

@wycats wycats closed this Jan 21, 2012

@krisselden krisselden reopened this May 4, 2012

Owner

krisselden commented May 4, 2012

Reopening because of e1ca514#L0R171 seems like a compelling use case.

Adding notes about addObserver roles:

addObserver(obj, [root, path], target, method)
obj: book keeper (fires events, keeps listeners and chains) responsible for destroy
root: root of chain
path: path of chain
target: this of listener
method: method of listener
arguments.length > 4: listener

Not sure I like the tuple param.

Owner

krisselden commented May 4, 2012

Would implement Ember.watch([root, path]) this add a chain watcher and fire change events for 'guid->path'

Owner

krisselden commented May 4, 2012

Also, if we allow for this on the to side of a binding it would allow for a separate bookkeeper from either the to or from. So in the case of keywords object, you could to([keywords, 'keyword').from([context, path]).connect(view) which would then be cleaned up by view.destroy() without having to ensure Ember.destroy(keywords) on view.destroy().

Owner

krisselden commented May 4, 2012

@wycats thoughts?

Owner

trek commented Jul 6, 2012

Can we move this to 1.1 milestone? We're trying to get a more manageable list for 1.0 release and this hasn't seen much attention.

Owner

wycats commented Jan 29, 2013

I'm going to close this because there are larger binding-related issues that will probably be addressed post-1.0 by @kselden and this hasn't really turned out to be a major blocker in practice.

@wycats wycats closed this Jan 29, 2013

@mozeryansky mozeryansky pushed a commit to mozeryansky/ember.js that referenced this issue Dec 30, 2015

@trek trek Merge pull request #217 from ryanhollister/patch-1
Update conditionals.md to show support for else if
3ee55ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment