Seems pretty reasonable to me, thanks Ryan
On Tue, Mar 8, 2011 at 12:23 PM, Ryan J Baxter <[email protected]> wrote:
> I started just adding to the .prototype directly but then I thought about
> x number of features doing this and decided it would be cleaner if they
> had the ability to add their specific functionality in their own
> namespace. Of course that only my opinion :)
>
> The only place I can think of to add a static function would be in
> shindig.container.utils. So maybe something like this, which would hide
> the use of the variable.
>
> shindig.container.utils.addContainerMixin = function(namespace, func){
> shindig.container.Container.prototype.mixins_[namespace] = func;
> }
>
> -Ryan
>
> Email: [email protected]
> Phone: 978-899-3041
> developerWorks Profile
>
>
>
> From: John Hjelmstad <[email protected]>
> To: [email protected],
> Cc: Ryan J Baxter/Westford/IBM@Lotus
> Date: 03/08/2011 02:31 PM
> Subject: Re: Allow extensions to the common container by allowing
> other features to provide their own namespace (issue4260049)
>
>
>
> Makes sense. Why again did you not want extension features to just add to
> .prototype directly?
>
> At the very least I'd suggest a "static" method to hide the mixins_ member
> variable. I'd prefer a method to a variable into which you stuff content
> since it defines the API better.
>
> So something like:
> opensocial.container.Container.addMixin = function(mixin) {
> // add mixin somewhere, which gets merged on ctor call
> };
>
> --j
>
> On Tue, Mar 8, 2011 at 11:04 AM, Ryan J Baxter <[email protected]>
> wrote:
>
> > We are in the process of writing several new features that would like to
> > extend the common container with some new functionality. But lets say I
> > am writing a new feature called foo. In foo's container code in the
> > feature I would have to do something like this
> >
> > shindig.container.Container.prototype.mixins_['foo'] =
> function(context){
> > return {
> > "fooMethod" : function(){
> > ......
> > }
> > };
> > }
> >
> > On my page, if I include the common container feature and my foo feature
> I
> > would be able to do something like this
> >
> > var container = new shindig.container.Container();
> > container.foo.fooMethod();
> >
> > The mixins method gets called by the constructor of Container and just
> > adds the methods to the Container object, but only if foo feature is
> > included on the page.
> >
> > Hope that makes sense.
> >
> > -Ryan
> >
> > Email: [email protected]
> > Phone: 978-899-3041
> > developerWorks Profile
> >
> >
> >
> > From: John Hjelmstad <[email protected]>
> > To: [email protected],
> > Cc: Ryan J Baxter/Westford/IBM@Lotus
> > Date: 03/08/2011 01:53 PM
> > Subject: Re: Allow extensions to the common container by allowing
> > other features to provide their own namespace (issue4260049)
> >
> >
> >
> > Well, what API did you want to expose for extension?
> >
> > By my (perhaps too quick) read of this code you'd have to create a
> > Container object, then do container.mixins_ = { bunch of mixins };, then
> > call containerObj.mixin_();
> >
> > In such case, may as well isolate state and just have the mixin method
> > itself take a method.
> >
> > --j
> >
> > On Tue, Mar 8, 2011 at 10:44 AM, Ryan J Baxter <[email protected]>
> > wrote:
> > John, I am not so clear on what your suggesting. I understand you don't
> > like the variable, but I don't see how you could execute a method to do
> > the "mixin" on the Container object when it has not been created yet. I
> > want a feature to be able to add it's own methods to the Container
> object
> > in its own namespace. So the mixins_ variable would only be used by
> other
> > features.
> >
> > -Ryan
> >
> > Email: [email protected]
> > Phone: 978-899-3041
> > developerWorks Profile
> >
> >
> >
> > From: John Hjelmstad <[email protected]>
> > To: [email protected],
> > Cc: Ryan J Baxter/Westford/IBM@Lotus
> > Date: 03/08/2011 12:59 PM
> > Subject: Re: Allow extensions to the common container by allowing
> > other features to provide their own namespace (issue4260049)
> >
> >
> >
> > Hi Ryan:
> >
> > The pattern of exposing a magical variable that's read when you call
> > this.mixin_() seems a little odd to me. I'd prefer to have something
> more
> > direct, ie have the mixin method itself take a method/function (which we
> > could define as being bound to "this" if you like).
> >
> >
> > --j
> >
> > On Tue, Mar 8, 2011 at 9:36 AM, Ryan J Baxter <[email protected]>
> wrote:
> > Anyone have any objections to this? I would like to get this submitted.
> >
> > -Ryan
> >
> > Email: [email protected]
> > Phone: 978-899-3041
> > developerWorks Profile
> >
> >
> >
> > From: [email protected]
> > To: [email protected], [email protected],
> > [email protected],
> > Cc: [email protected], [email protected]
> > Date: 03/04/2011 11:36 AM
> > Subject: Allow extensions to the common container by allowing
> other
> > features to provide their own namespace (issue4260049)
> >
> >
> >
> > Reviewers: mhermanto, dev_shindig.apache.org,
> > dev-remailer_shindig.apache.org,
> >
> > Description:
> > At the moment the common container cannot be extended by adding a new
> > namespace and still have access to the container object. The only
> > reasonable way to extend the common container at this moment it to add
> > new methods by calling .prototype or by subclassing it. Another useful
> > extension would be to allow features to add their own namespace to the
> > common container and add new methods under that common container.
> > However those methods may still want to reference other methods inside
> > the common container so they need to be able to access the common
> > container object itself.
> >
> > Please review this at http://codereview.appspot.com/4260049/
> >
> > Affected files:
> > features/src/main/javascript/features/container/container.js
> > features/src/test/javascript/features/container/container_test.js
> >
> >
> > ### Eclipse Workspace Patch 1.0
> > #P shindig-project
> > Index: features/src/test/javascript/features/container/container_test.js
> > ===================================================================
> > --- features/src/test/javascript/features/container/container_test.js
> > (revision 1078023)
> > +++ features/src/test/javascript/features/container/container_test.js
> > (working copy)
> > @@ -112,6 +112,22 @@
> > this.assertTrue(container.sites_[2] != null);
> > };
> >
> > +ContainerTest.prototype.testMixins_ = function(){
> > + this.setupGadgetsRpcRegister();
> > + shindig.container.Container.prototype.mixins_['test'] =
> > function(context){
> > + return {
> > + "getSitesLength" :
> > function(){
> > + return
> > context.sites_.length;
> > + }
> > + };
> > + };
> > + var container = new shindig.container.Container();
> > + this.setupGadgetSite(1, {}, null);
> > + container.newGadgetSite(null);
> > + this.assertTrue(container.sites_[1] != null);
> > + this.assertEquals(container.sites_.length,
> > container.test.getSitesLength());
> > +};
> > +
> > ContainerTest.prototype.setupGadgetSite = function(id, gadgetInfo,
> > gadgetHolder) {
> > var self = this;
> > shindig.container.GadgetSite = function() {
> > @@ -137,4 +153,4 @@
> > register: function() {
> > }
> > };
> > -};
> > +};
> > \ No newline at end of file
> > Index: features/src/main/javascript/features/container/container.js
> > ===================================================================
> > --- features/src/main/javascript/features/container/container.js
> > (revision
> > 1078023)
> > +++ features/src/main/javascript/features/container/container.js
> (working
> >
> > copy)
> > @@ -113,6 +113,8 @@
> > * @private
> > */
> > this.tokenRefreshTimer_ = null;
> > +
> > + this.mixin_();
> >
> > this.preloadFromConfig_(config);
> >
> > @@ -121,7 +123,6 @@
> > this.onConstructed(config);
> > };
> >
> > -
> > /**
> > * Create a new gadget site.
> > * @param {Element} gadgetEl HTML element into which to render.
> > @@ -636,3 +637,18 @@
> > }
> > });
> > };
> > +
> > +/**
> > + * Adds the ability for features to extend the container with
> > + * their own functionality that may be specific to that feature.
> > + */
> > +shindig.container.Container.prototype.mixins_ = {};
> > +
> > +/**
> > + * Called from the constructor to add any namespace extensions.
> > + */
> > +shindig.container.Container.prototype.mixin_ = function(){
> > + for(var i in this.mixins_){
> > + this[i] = new this.mixins_[i](this);
> > + }
> > +}
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
>
>
>
>
