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

Use Function.bind() or lang.hitch() equivalent? #4

Closed
wkeese opened this Issue Mar 5, 2014 · 6 comments

Comments

Projects
None yet
3 participants
Owner

wkeese commented Mar 5, 2014

As an example, see the preCreate() method in the current version of deliteful/ViewStack.js:

preCreate: function () {
    ...
    this.on("DOMNodeInserted", this._setChildrenVisibility.bind(this));
},

The bind() call for _setChildrenVisibility precludes the app from attaching to the _setChildrenVisibility() method, for example:

require(["deliteful/ViewStack", "dcl/advise"], function(ViewStack, advise){
    var vs = new ViewStack();
    advise.after(vs, "_setChildrenVisibility", function(){
        console.log("function called");
    });
});

This is because bind() does eager binding, rather than lazy binding.

The alternative approaches [when setting up binding during the widget creation lifecycle] are to:

(1) use dojo/_base/lang.hitch() or a similar method from our own utility library

preCreate: function () {
    ...
    this.on("DOMNodeInserted", lang.hitch(this, "_setChildrenVisibility");
},

(Note that we can make our own utility library if we do not want to introduce a dojo dependency.)

(2) hardcode a closure

preCreate: function () {
    ...
    var self = this;
    this.on("DOMNodeInserted",  function(){ self._setChildrenVisibility(); })
},

Note that Function.bind() can still be used in other cases, after widget creation has finished.

I like the idea of just using bind() but I'm starting to think it's too restrictive to our users.

cc @pruzand, @cjolif

@wkeese wkeese added the question label Mar 5, 2014

asudoh commented Mar 7, 2014

Unless we want to let application developers set an advice to a prototype function, we may set the bound function to an instance property and let application developers set an advice to such instance property, like:

preCreate: function () {
    ...
    this.on("DOMNodeInserted", this._boundSetChildrenVisibility = this._setChildrenVisibility.bind(this));
},

...

require(["deliteful/ViewStack", "dcl/advise"], function(ViewStack, advise){
    var vs = new ViewStack();
    advise.after(vs, "_boundSetChildrenVisibility", function(){
        console.log("function called");
    });
});

Whether we want to do that may depend on whether we foresee many use cases setting an advice to something like _setChildrenVisibility(). If we foresee many use cases of that, I'd prefer 2).

Owner

wkeese commented Mar 7, 2014

Unfortunately I don't think your this._boundSetChildrenVisibility will work; it's still doing eager binding.

I feel like dojo users did a lot of dojo.connect() calls for the widgets although I forget for what exactly.

asudoh commented Mar 7, 2014

Right my _boundSetChildrenVisibility() idea was premature.

Most of non-event-listener dojo.connect() usages I can see in legacy Dojo projects I've involved in are custom event alternatives (dojo.connect(ob, "onCustomEvent", "customEventHandler");) where ob can (directly) call onCustomEvent() instead of creating an (actual) custom event and dispatching it.

Not sure if there are any applications that use dojo.connect() in AOP context extensibly.

Owner

cjolif commented Mar 21, 2014

At least when the method is not publicly documented (like the one you are referring to) I think we should not care about it and just use bind. For documented methods that's indeed a bit different. For documented methods we might want to use the closure solution (2) for now with a self variable.

Owner

wkeese commented Apr 8, 2014

Sounds like a plan, don't forget to add to https://github.com/ibm-js/sdk/blob/master/GUIDELINES.md.

Owner

cjolif commented Apr 8, 2014

closed in 5236d5e

@cjolif cjolif closed this Apr 8, 2014

@cjolif cjolif referenced this issue in dojo/gfx Apr 9, 2014

Open

get rid of lang.hitch #13

@cjolif cjolif added a commit to cjolif/deliteful that referenced this issue Apr 9, 2014

@cjolif cjolif implements ibm-js/sdk#4 3e6f9f0

@wkeese wkeese added a commit to ibm-js/delite that referenced this issue Apr 11, 2014

@wkeese wkeese stop using lang.hitch() in delite, refs ibm-js/sdk#4 1f321b8

@wkeese wkeese added a commit to ibm-js/delite that referenced this issue Oct 23, 2014

@wkeese wkeese stop using lang.hitch() in delite, refs ibm-js/sdk#4 cf42172

@cjolif cjolif added a commit to ibm-js/deliteful that referenced this issue Feb 6, 2015

@cjolif cjolif implements ibm-js/sdk#4 f95fdd4

@wkeese wkeese pushed a commit to wkeese/deliteful that referenced this issue Mar 12, 2015

@cjolif cjolif implements ibm-js/sdk#4 45fbf53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment