Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Phase out bindContext #74

Open
Tracked by #69
creynders opened this issue Sep 24, 2014 · 12 comments
Open
Tracked by #69

Phase out bindContext #74

creynders opened this issue Sep 24, 2014 · 12 comments

Comments

@creynders
Copy link
Contributor

First of all: bindContext seems to contain only view-related stuff anymore. Maybe it makes more sense to move this to when a view is wired/instantiated.
Second: I don't see any advantage of it being a static separate function anymore, or am I missing something @geekdave ?

@creynders creynders mentioned this issue Sep 24, 2014
16 tasks
@creynders
Copy link
Contributor Author

@mmikeyy I did find a use for bindContext, but ATM it's not really optimized to do so: allowing the wiring of a class to a context, yet binding its instances to another context. This opens up the possibility of being able to wire views to a context, yet coupling the view's instances to another (child) context.

@mmikeyy
Copy link
Contributor

mmikeyy commented Oct 6, 2014

Ahh!! Hmm... you're catching me in the middle of a rewrite of an old application (from my pre-Geppetto, Marionette, Backbone era, years ago, when I thought I was some sort of Javascript artist, but I was actually just enthusiastically ignorant! The app is cool, though, from a user's perspective, that is, as long as one doesn't lift the hood...)

Anyway, all that to say that I'm taking this opportunity to include all the goodies I know of in this rewrite. Including Geppetto.... without bindContext!

I can understand now your puzzlement when I frowned at the announced phasing out of this function. Now that I'm not using it, I tend to find it difficult to see why one would really need it at all.

What you are describing seems to be what I was doing with the other application (that I've had to set aside for now) that I'm working on (first Geppetto integration for me). View B was wired to view A's context A. Upon instantiation, bindContext would bind view B with its own context B, declaring context A as parentContext.

In order for this to work, I need to set view B's wiring in @wiring in the initialize method, NOT in the view's prototype because in the latter case, context B is not yet instantiated and therefore references to it when it is wired from context A would be unresolved.

Anyway, I figured that this was the natural way to go with Geppetto after reading the documentation, which presents bindContext without letting the reader suspect that it's considered useless and about to be thrown away by the powers that be!

I'm not advocating a return to grace for bindContext, now that I discover how easily I can live without it. But when I return to the other app I'm working on, it's possible that I'll understand again how it makes my life easier when Geppetto is added to an existing application. It's a bit difficult, when one is well entrenched in a certain frame of mind (+ lack of sleep...:sleeping:), to easily see the world from another point of view.

Something bindContext tends to do perhaps is to provide new users with an easy (perhaps too easy) path to a one context per view application structure. When parentContexts were fixed, I was happy to find what I was looking for: easy reference to data anywhere up a view's ancestry, without having to declare anything global or transfer data payloads in initialization parameters. But a problem eventually appears: I can refer to data that can be pretty far away in a snap of a finger without having to bother knowing where exactly it comes from. But often, I do have to go see where it comes from and I find myself in a maze of contexts, sub-contexts, sub-sub-contexts etc and this drives me crazy.

At one point, using bindContext became second nature to the point that I could even have a collection view with hundreds of lines, each one bound to its own context. Granted, these contexts would not contain anything other than a few commands, but why instantiate it so many times? Well... it just seemed natural and so easy. bindContext is there, let's use it!

One thing that would be good eventually perhaps would be to have some kind of cook book or at least more examples aimed at illustrating different use cases. Too often, examples are presented that are aimed at showing programming prowess rather than easily understood solutions to common problems. I remember an example somewhere that showed Geppetto code, and the writer wrote something like 'ha! and now you're wondering how this code can accomplish anything?? Now let me show you the secret!". And he takes his puzzled reader to some remote javascript file where in just a few lines, the magic is claimed to be unlocked. I find that too often, examples are in a programming style that is so polished that they become enigmas for the casual reader, and in the end, they miss their point which should just be to illustrate a concept as clearly as possible...

Well, I've done it again. Really can't stop writing once I get going...:grimacing:

So to summarize:

  • yes, I think there can be a use for bindContext;
  • but: it should not be just thrown there in the documentation without guidance on different ways to structure an application with simple examples, as this function might send a new user down the wrong path initially, judging from my own experience (but who knows, perhaps that's just me after all...:confused:). I'd like to point out in passing that my second Geppetto experience (without bindContext) is much more enjoyable than the first, which generated an astronomical number of source files...

@creynders
Copy link
Contributor Author

Now that I'm not using it, I tend to find it difficult to see why one would really need it at all.

Ah, cool!

Something bindContext tends to do perhaps is to provide new users with an easy (perhaps too easy) path to a one context per view application structure.

I agree. It's too easy. And misleading. It should be used only in very specific use cases, not by default.

One thing that would be good eventually perhaps would be to have some kind of cook book or at least more examples aimed at illustrating different use cases.

Agreed. I'd really want to expound on why using Geppetto is a good idea. What problems it solves that are harder to solve w/o it.

this function might send a new user down the wrong path initially

Yes it does. I think part of the problem is that it's a class member as well. Which it doesn't have to be. I'd definitely "promote" it to an instance member of Context. Also, it does too much and too little at the same time, it's called bindContext but

  • it does more than that (it aims to destroy the context when a view is closed, but that doesn't even function)
  • in its current implementation it's meant to be used on views only, while the name does not reflect that.

Anyway, I still need @geekdave 's feedback on this (I think he's a bit overwhelmed with work right now). Maybe I'm missing something important here.

@mmikeyy
Copy link
Contributor

mmikeyy commented Oct 8, 2014

@creynders , I'm running into a problem that seems simple and that I've solved in a way that works but is not ideal at all.

Suppose I have a collection that must have a model class. With Geppetto, it seems that the way to go is

// context.js
context = Geppetto.Context.extend({
        initialize: function() {
            this.wireValue('model_class_wired_as_value', model_class)
        }
    }

);

// collection.js
collection = Backbone.Collection.extend({
        wiring: ['model_class_wired_as_value']
        ,
        initialize: function(){
            this.model = this.model_class_wired_as_value
        }

    }
);

The reason this seems to be the way to go is that the collection prototype needs a model prototype (not a model instance...).

The problem with this however is that values are not wired up.They're just plain values.

A model may need to access data through wiring, but it can't when wired as a value. It's accessible for other wired elements, but it's a dead end: it can't access anything else.

Wiring a model as a class won't work either as this produces instantiated objects only.

The only way that I see to wire a class and still have it available as a class is to wire it as a view. It just works. Even for models or anything else probably.

So my feeling is that perhaps it would be a good idea to simply come up with an other term for wireView that would be functionnally identical, but would better describe what is being done. Perhaps a more general term that would leave wireView untouched and available both for backward compatibility and for the eventual addition of view-specific functions. Unfortunately wireClass is taken, so I'm thinking of something like wireRawClass, which is short and conveys the idea that the class will not be transformed into something else...

As much as I like when my program works, I hate to have to write this.wireView a_model .

Am I missing something here?:scream:

@creynders
Copy link
Contributor Author

Am I missing something here?

No, you're not. This is why I added a wireFactory method in #77 wireView is basically an alias to it in the current version. (Later on: these two will diverge however)
I'm stacking some changes in a patch branch (just as you suggested; to collect the changes in a separate branch) to allow Dave to review my proposed changes easily. There's a few more things I want to add to it before we release 0.7.2
In the mean time you could monkey patch it yourself by attaching a wireFactory method to the Geppetto.Context, something like this:

Geppetto.Context.prototype.wireFactory = function(key, clazz, wiring){
    return this.wireView(key, clazz, wiring);
}

Then when v0.7.2 is released all you have to do is remove the monkey patch.

@creynders
Copy link
Contributor Author

BTW I'm really interested in your opinion on the fluent syntax, feel free to comment on it in PR-73 I really enjoy our discussions and find them very valuable to get a better understanding on how things work (best) myself.

@mmikeyy
Copy link
Contributor

mmikeyy commented Oct 8, 2014

Ahh! Excellent! I'll gladly take any function you provide for this, but I just find its name a bit surprising. (especially after our discussion on stopPropagation vs propagationDisabled...!)

At the present time, we have:

  • wireValue which ... wires a value.
  • wireClass which ... wires a class, but also instantiates it
  • wireView which... wires a view without instantiating it, but transforms it into a factory.

So we have one verb and different complements, and in each case the action is different.

It is also possible (and I do most of my wiring this way) to use a wiring object, with ...

  • values: {...}
  • classes: {...}
  • views: {...}

where each member does the same thing as its method counterpart.

Now, if we add wireFactory, we have again verb + complement but... are we not adding to the existing confusion (same verb presently producing different results for no apparent logical reason, especially for views vs classes - it's just a design choice) by introducing the same syntax, with as a complement a term that cannot logically be acted upon by the verb? To me, wireFactory sounds a bit out of place among wireValue, wireClass, and wireView. We're certainly not wiring a factory, but does it not sound like we're trying to do that?

Another problem is that I don't see an interesting term for use in the wiring object. Would we have values, classes, views, and factory??

I thought at one point that wiringFactory would possibly be better than wireFactory, because ... it's what it is. But I was a bit disturbed by the lact of an action term, meaning that it describes what it is, not what it does (contrary to the terminology used for all other wiring methods). So finally, I saw light and I'm tempted to propose this:

  • wire: (yep, just that) as a general purpose wiring method (equivalent to the current wireView method, which will eventually be allowed to diverge to become more view-specific);
  • the other existing flavors, which will each have its specificity.

I'd go even one step further, and make it more basic than the current wireView method by removing the factory wrapping that renders prototype members unreachable in the non-instantiated wired class. This way, we'd know that when we wire a class, we're getting just that, a wired class, not some extra stuff that in certain cases, we might not want.

In the rare cases when I need to inspect the prototype of a wired class before instantiation, I would know that if I wire the class instead of wireViewing it, I get plain-vanilla wiring that will not interfere with my logic. So this means that wireView would already be customised for views instead of being initially identical to wire.

Finally, this leaves open the problem of the wiring object. We can't have:

wiring: {
  nothing: {},
  values: {},
  classes: {},
  views: {}
}

I stop here because I don't want to waste too much time if - who knows - I'm missing something big that is obvious to everyone but me here!

I'll have a look at the fluent syntax thing you mention in the other post as soon as I have a chance. I glanced at it already but I did not have the time to really understand it enough to participate in the discussion.

@creynders
Copy link
Contributor Author

but I just find its name a bit surprising

But that's exactly what you get back from the context in the current implementation ... a factory: a function which creates an instance of a class. Granted, out of convenience it allows for creating instances with new as well, but that doesn't make it any less of a factory. In this case, all views happen to be factories as well, but that's just coincidental.

I have the most trouble with wireClass, since it's the least "truthful" method name, but wouldn't know how to call it otherwise.

by removing the factory wrapping that renders prototype members unreachable in the non-instantiated wired class

Hehe, it really bothers you. I'll think about how we could allow it to be optional.

@mmikeyy
Copy link
Contributor

mmikeyy commented Oct 8, 2014

But that's exactly what you get back from the context in the current implementation

yes, but the method name wireView does not give any indication that a factory is returned. I'm just pointing out that we have methods wireSomething (formed as verb-complement) that operate on different things, setting up the wiring as would be expected, but also adding some creative things that for some reason may sometimes be unwanted. Strangely, just plain wiring is not an option! (except for a dumb value). And perhaps it's just me, but if we throw wireFactory in the lot, are we not breaking the consistency of the method naming scheme used so far? (i.e. "name = verb + object type operated on", not "name = what the method returns")

I must concede that I have come to like these factory methods for views. All that bothers me is to have them imposed on me with no way of avoiding them.

So... ok, perhaps we could have...
wire for plain vanilla wiring (no factory method, just basic wiring) (yeah... not giving up on this one yet!)
wireSomething for the most common use cases of the different 'somethings'
and...
I've glanced shortly a while ago at the discussion on fluent syntax (not enough to say anything intelligent about it I'm afraid...) and I got the impression that someone was proposing jQuery-style chaining, and if it is the case, I think it might be interesting to be able to construct all wiring flavors as a set of basic chained operations such as:

context.wire(name, obj) for plain vanilla wiring (yep! here it is again! perhaps you'll come to like it if you keep seeing it!! 😄)

context.wire(name, obj).factory() for plain vanilla + return a factory function

context.wireView(name, obj) for views, which would initially be equivalent to
context.wire(name, obj).factory(), but might eventually evolve to

context.wire(name, obj). viewCandy1(option) .viewCandy2(option).factory()

context.wire(name, obj).instance([configure]) would create an instance, with optional initialization parameters,

context.wireClass(name, obj) would be short for the above, but could also eventually get some special class candy just like the views.

Another idea: to be compatible with the current naming scheme, wireFactory should rather be named wireAndReturnFactory, which is atrocious. How about wire2Factory implying that we're wiring,and returning a factory.

Ahh! Ideas keep popping up today! Since you don't seem to like my plain wire method, hmm... let me propose wireAndDoNotReturnAFactory!!! 😛

[just edited out a paragraph that I had added in the heat of the moment, before I remembered that the chained methods are collectively used as a command, not as a function expected to return a value...]

Anyway, I'm stopping here for lack of time (and other things to do!). I'm possibly stealing someone else's ideas that are resurfacing from my subconscious because of that quick glance I had at the other conversation several days ago. If that's the case, no problem: I'm not seeking credit for anything that might be good here. Except for... my plain vanilla wire method!:smirk:

@creynders
Copy link
Contributor Author

are we not breaking the consistency of the method naming scheme used so far? (i.e. "name = verb + object type operated on", not "name = what the method returns")

But the naming scheme is "name=what the method returns". wireSingleton returns singletons, you don't "register" singletons; you register a class and the context returns a singleton. The same applies to commands and values. That's why I don't like wireClass, since it doesn't fit the naming scheme. wireView is debatable. wireFactory on the other hand does fit the original naming scheme.

All that bothers me is to have them imposed on me with no way of avoiding them.

Yeah, fair enough. Let's fix that in 0.8

wire for plain vanilla wiring (no factory method, just basic wiring) (yeah... not giving up on this one yet!)
wireSomething for the most common use cases of the different 'somethings'

That's confusing, inelegant and doesn't communicate intent well IMO. However, I think you'll be glad to see that the basis of the entire fluent interface in #73 is wire. 😁 I think it's best you take a look at it, since it proposes some ideas similar to what you propose here.

@mmikeyy
Copy link
Contributor

mmikeyy commented Oct 9, 2014

Ouch!! I had completely lost sight of the singletons! You're right; they send crashing and burning my understanding of the current naming scheme (i.e. current scheme = verb + what it operates on). For me,wireView meant "wire this view class for me please" (polite form! 😉), wireCommand meant "wire this command", wireValue meant "wire this value". It all made sense as I'd get a wired view class (until it was turned into a factory...), a wired command and a wired value. Now I had to remember that if I wanted a class wired, wireClass would not only wire it, but instantiate it at the same time. As you point out yourself, it does not fit the "wire + what-you-get" naming scheme either because we're not getting a class, but an instantiated object. And finally, I admit that wireSingleton really had no place in the naming scheme as I understood it. But it was just another strange animal in a strange world; I understood what it did (and did well) and did not pay too much attention to its name.

So OK!! I understand now that wireFactory *is * consistent with the naming scheme. All that might be missing now perhaps is some kind of wireAsIs method to just wire without returning the object transformed in other unwanted ways. (arghh! ok, I know, it's my old wire in disguise!! 😊)

@creynders
Copy link
Contributor Author

I realised I have neglected so far to mention the main reason why constructors are wrapped in the first place: Backbone Collections (and Marionette CollectionViews) maintain their children themselves, e.g. fooCollection.add({foo:"foo"}) will create an instance of its model class. To allow these instances' dependencies to be resolved we need to wrap the constructor with the injection call, otherwise it would have to be done manually (which would be a real pain). Now, this has always been the case, even in v0.7.0 but it used to be a wrapping of initialize instead of constructor. So actually, you never received a vanilla class instance, it was always wrapped. (You just didn't know 😉 )
The reason it changed from initialize to constructor was threefold: 1/ it allowed me to add the self-instantiation-bit to it 2/ it works on non-Backbone objects as well 3/ I'd rather let the system be "honest", i.e. you immediately see that what you get is not what you put in there.
Now, the use cases where it's not beneficial to have the constructor wrapped are IMO far more rare (and easily avoidable, nay, even preferably avoidable) which warranted the wrapping due to its ease-of-use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants