-
Notifications
You must be signed in to change notification settings - Fork 28
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
Backwards compatibility breaks in 0.7.1 #75
Comments
Yes, the execution sequence of
makes more sense. Heheh @geekdave 's comment was:
I agreed. 😁 Lesson learned:
(let's call this camille's law from now on 😉 ) |
This context.wireView('view', 'FooView');
//1. as a factory method
var factory = context.getObject('view');
var view = factory(); //an instance of FooView is created
//2. as "class"
var View = context.getObject('view');
var view = new View(); But I tested it pretty extensively and it definitely was meant to be backwards compatible. I upgraded my own projects at work to v0.7.1 (before it was released) without any problem which is why I was pretty confident that backwards compatibility would not break... I'm really curious why this does break your code. |
OK... I'm back at it after sleeping on this. The reason the code breaks seems to be as follows. I rely on examining the view class prototype to determine the kind of class received (by a view, from the execute method of a command that has wiring:['viewThatWillBreakCode']) and to make decisions on how to process it (what layout and/or template to use, what data to supply for initialization) before later instantiating it. Now it seems that instead of my class prototype, I'm getting this factory method. So there's no way to know what it is before instantiating it (or is there?), which I don't want to do just yet. It's easy to work around the problem (at the expense of elegance... need to send class + tag identifying class type, instead of the class carrying its own type in its prototype for everyone to see). I hope I'm making sense. I just woke up 😪 and it suddenly seemed so obvious (and @creynders is so curious!). I'll revisit the issue later this morning and make corrections if needed. I will also change my code and see if it can be fixed as described, without new unforeseen issues arising. |
That kind of defeats the purpose, no? Whether the command sends a string "fooView" or it sends the prototype doesn't matter, since your receiving view still has a dependency on the fooview class, i.e. you might as well send a string instead, inspect that and construct the view instance there. In my experience this kind of switch-views are a bad idea, they grow monolithic, know too much about the components their initialising and have dependencies all over the place. It's far better to separate the construction and initialisation of each view into a separate factory. Anyway... There are obviously solutions to the need of knowing the type of what is sent through. For instance I could expose a |
Well... the So I have a collection of items of different kinds displayed. When an item needs to be edited, an event (specific to this event type) is dispatched to parents. Somewhere up in the context ancestry, a context handles all editing requests. It opens a layout view that's initialized with model, parentContext and the class that should be instantiated in a layout region. Some very basic decisions (no arcane switching logic here) such as what template to use are made by looking at the class received. All classes used for this have a "type" property for this purpose. I don't think there's any bad programming practice here. And if a view constructor must be passed around, isn't it always better to have it contain its own defining properties (e.g. a type) rather than having this info sent along in a separate parameter? But the solution is very simple. I can send a string to the layout view instead of the view prototype, as you suggest. The layout view will set itself up based on the string, show the view in the appropriate region, this view being simply obtained by Ahh.. but I thought that Of course, I could dispatch locally to get a command (with context access) to obtain the view with getObject, and then send it back to the layout by triggering a contextEvent or returning it in the payload. But all this triggering back and forth makes the logic difficult to follow when what needs to be achieved is so simple. Perhaps it's less trouble to send the class prototype to the layout view along with the type as a separate parameter after all... (or keep making the context available to the view, as I'm in the habit of doing, and use If you see me missing an obviously better and easier way of achieving the same result (or saying something completely idiotic 😕 ), do not hesitate to enlighten me!
Not sure about this. May be too specific to the use case we are discussing. Next thing we'll see is someone asking you to expose something else...
Ahh... that would be a dream... It's a bit frustrating to see my own stuff wrapped to the point that I, its creator, am denied access to its contents! |
Disclaimer: we're entering opinions-land here, based upon higly individual experience, so what follows is exactly that; opinions. First of all:
There's plenty of things that are "best" or "usual" practices in Marionette that I dislike, which is why I like Geppetto so much, it helps me to avoid a bunch of those things.
There's a (big) difference between a) having a single view spawn different templates with
The way I use layouts is always w/o any kind of control logic in the layout view. It simply listens to context events and switches views accordingly. The only "controlly" decisions it can make are decisions like: "on event A show views V1 and V2 in regions R1 and R2" and "on event B close regions R1 and R2 and only show V3 in R2" i.e. layouts are allowed to make decisions on which views are shown on which events, but I never ever let a view vary its presentation upon anything else than different events.
Obviously it's a bit hard to tell without knowing the exact use case or requirements, but sounds to me like that single view should actually be split into several views. But even if that's not the case, there's plenty of ways to circumvent such logic, for instance: you could create a template/view mapping somewhere and inject it into the layout view, then all it has to do is lookup the template using the view constructor (yes this will work even with wrapped constructors, as long as those were used in the template/view mapping as well) or inject the layout view with factories that will create the view instance and return the correct template to the layout view or ... it all depends on what exactly needs to be achieved.
I'd say "yes", I don't think it's a good idea to pass two defining values.
Inside views? Definitely. Then you switch from using DI to the service locator pattern. It's not totally unavoidable, but definitely has no business being in views.
Yeah, I understand you want to keep the layout view ignorant of the views it receives. However, having a switch-statement is an implicit declaration of dependencies, i.e. you might as well (or better even) declare them in
Insane, I say, insane! 😉 Again, it's a bit hard w/o seeing the code, but sounds to me that you need another set of views.
Yeah, maybe. I need to think about that. |
opinions... yes, of course. And I hope I'm not coming out as too much of a whiner... 😳 : At any rate, my application is fixed now and I'm moving forward from here. Finally, I had only two problems and the fix was quick and easy. Thanks for the numerous comments: it gives me ideas on ways to improve my code. I remain a bit puzzled by a few things such as the announced phasing out of bindContext (ouch! I use it in every single view! must eventually figure out how to live without it!) or the wrapping of view constructors that makes their prototype unreachable (no big deal, btw. I frowned a little when that broke my app, but no one ever died of frowning after all..). When I'm finished converting this application I'm working on, I'll need to make another pass to improve a few things. Hopefully, Geppetto won't have evolved to the point of making a complete rewrite necessary! 😉 Thanks again. |
@mmikeyy it seems like I can fix it, i.e. you could still use your As you can see Would this suffice? |
Why exactly ? For the automatic closing of the view? Or simply for the automatic injection of |
I see that the POC works, but if I implement the same change in Geppetto, it does not work. It may be due to Anyway, I find that it works well with this instead:
I don't know if there would be unanticipated side effects. The advantage is obviously that the resulting object can be inspected for certain characteristics, but someone could lose sight of the fact that anything that he sees in that object is just a prop that will be cast aside when the class is instantiated (so don't change anything before instantiating: it's useless. Upon instantiation, original prototype properties/methods will come back and overwrite any changes made). I guess the documentation should mention something about this, especially for users who expect to be able to use the library without reading and understanding the code behind. What is the advantage of the factory function anyway? I'm sure it's not there just for the pleasure of not having to type In any event, I'm not sure it's worth the trouble to load the object with props, if that's all they are (props), given the risk of confusion later when someone thinks he's handling his own object, while actually it's been secretly taken away from him and he's just playing with a copy. For that reason, I think it's better if I play it safe and just send the view type as a separate parameter. And as a trade-off, I guess I'll be able to enjoy the benefits of a factory method when I understand what they are (yeah... I know... I'm not the most sophisticated programmer around here 😕 ).
Hmm... automatic closing of views? no... you probably mean automatic destruction of context upon vlew closing. That's one thing. BTW, I extend the Geppetto prototype to react to a view being destroyed, not closed. Marionnette no longer closes views (Unless I'm mistaken, I don't think ok, what else... well it's probably just ignorance on my part, but I read the documentation, it was there, and I just started using it. I found a way of doing things that worked well for me and I just kept going. Not a problem to adjust if But here's my standard way of doing things: (for each view, I have 3 files. I don't like the idea of one file per command for example: too many files to handle, too many tabs in IDE). I name them in a way that makes it obvious that they're related. // file example_CMD.js
define([list],
function (list) {
return ({
command1: function () {
},
command2: function () {
}
}
)
})
// file example_CNTXT
define(['geppetto', 'example_CMD', etc],
function (Geppetto, cmd, etc) {
return Geppetto.Context.extend({
initialize: function (data) {
data.view.context = this; // in case the view needs it
// + some wiring done manually
},
wiring: {
commands: {
command1: cmd.command1
// etc...
}
}
}
)
})
// file example_VIEW
define(['marionette', 'geppetto', 'example_CNTXT'], function(Marionette, Geppetto, context){
return Marionette.ItemView.extend(
{
initialize: function(opts){
this.wiring = [list]
Geppetto.bindContext(
{
view: this,
parentView: opts.parentView,
context: context
}
)
// this.context available from now on if ever needed
}
}
)
}) I hate these parentheses and brackets... Coffeescript is such a pleasure to work with... Ok... now from the wiring: ['example_VIEW']
execute: function(){
new this.example_VIEW({
parentContext: this.context
})
} Pretty basic and boring, but... you asked for it! You can throw 🍅's now... What is the "recommended" way of doing things now? |
Ah, so
Simply to leave out the
I agree. Either I'll drop the constructor wrapper or we leave it as is.
Yes, that's what I meant.
Well, there are as many ways as there are developers. A simple example of how I handle views can be found in the geppetto todomvc app In a nutshell: I configure their dependencies outside the views themselves, in this case in BootstrapUI If you want a more "real" example, this is the project I'm working on ATM. It's nowhere near completion, so there's tons of things that need to be refactored to proper best practices. I tend to write the code to get things done and then when I'm certain that's the approach I'll be taking, I refactor. |
nope... it is static. (was actually, now that I've given up on inspecting the prototype to identify the class type before instantiation. No big deal: there are many ways to obtain the same result.) Thanks for the comments. In the apps you mention, unless I missed something, you seem to have a single context to which everything is wired. Of course, if you were to use bindContext in a view here, you'd have a problem with this shared context being destroyed with the closing of any view linked to it this way. Also, the code seems to have been written with Geppetto in mind right from the start. The show really seems to be run from the context, with very dumb views just being told what to do from a distance (the name "Geppetto" being very appropriate here). This way of structuring the app may not be easily implemented when Geppetto is integrated to an existing application. With bindContext, we can add the context and some wiring without disturbing the existing control flow. The problem then is that it allows one to become comfortable with a way of doing things that is probably not optimal. Perhaps this is the good thing about bindContext: it provides an easily understood and implemented way of integrating Geppetto to an existing app with very little rewriting required. Is this making sense?? Anyway, I'll try to get rid of it in some parts of my application just to see how painful it is to do this... |
Sorry, I don't mean to be overly pedantic or autistic, but want to get to the bottom of this, because if you're right then something's horrible wrong with how the factories are created. How are you attaching the Backbone.View.extend({
type : "FooView"
}); And how do you check it? Like this: switch(viewClass.prototype.type){
} If you use any of these, then it's not a static, but an instance property.
Yes, I have a single context per bundle. I.e. everything that's concatenated into a single js file has one context. And granted, that's definitely one of my blind spots, I tend to see everything from the single-context POV.
Yes and no. I knew right from the start I'd be using Geppetto for this project, but I am writing all actors (views, models and service) to be Geppetto-independent. This is a 3-year project and I have no idea how it'll evolve in the next years, so it needs to be as flexible and adaptable as possible. For instance, most probably I'll be creating a Titanium app as well, which uses its own Backbone like framework (Alloy). However I have no idea how easy it would be to add/adapt/port Geppetto to it, which is why my actors need to be as independent as possible. A long winded way of saying that if I weren't using Geppetto my actors would look (almost) exactly the same. And TBH that's what one should aim for. I can't ditch the direct dependencies on Backbone and Marionette unfortunately (due to their bad architecture I'm afraid to say) or at least can't ditch them w/o jumping through ridiculous hoops, but if I could, I'd definitely would.
That's an interesting point you raise. Yesterday evening I took the time to look at some of my past projects that don't use Geppetto and tried to estimate how much work it would be to add Geppetto to them and came to the conclusion that in almost all of them it would be pretty trivial, but probably that's because in this chicken-and-egg scenario (what shapes what? Does the usage of Geppetto shape the way I build my projects or the other way around?) I'd say the chicken definitely came first, i.e. the way I architect my projects shapes my contributions to Geppetto. So, coming back to And @mmikeyy I really want to thank you for these conversations, they're hugely useful both as feedback on the usage of Geppetto, but also because they make me rethink and question my habits. |
Don't be so humble! Your the knight here and the humble part is mine! 😉 Ok, let's see if I ridicule myself with some misconception. 😳 First, I never use This being said, I may be wrong (possibly because of something I never fully grasped in JS's prototypical inheritance model), but I've always considered any extended class (among which classes extended via the Therefore in my (simple - but it's worked so far!) view of the world, any property extending a class with the Was I using this?
yes, originally, with Geppetto 0.7.0. But it no longer worked with 0.7.1. And yes, in Coffeescript: class viewClass extends Marionette.ItemView
type: 'whatever' in Javascript: viewClass = Marionette.ItemView.extend({
type: 'whatever'
}); Anyway, that's it. I don't see why Concerning my attempt at removing bindContext from my class initialize method to prepare for its eventual disappearance, I've spent a few hours on this and it's not necessarily so obvious (but I was tired that day and perhaps I'll do it in the snap of a finger when I'm back at it). I must say that Anyway, I'll finish the job of getting rid of
OK, I know that you're humbly conceding a blind spot here, but I can't resist mentioning that I found this quite surprising!! All components are carefully designed to be this and that-agnostic etc, but they are designed with the source files in mind!? 😛 Happy to read that you're getting something from these conversations too! |
What you don't know, you don't know. There's no shame in that. For example: only last week I figured out why many
But you do. 😄 This: class viewClass extends Marionette.ItemView is exactly that. Wel probably, not exactly, but it's the same thing. You copy the prototype of
That's correct. The only misconception is in the terminology I think.
They're called "instance properties" since that's the scope in which they are to be accessed. In CoffeeScript "static members" (or using the more proper term "class members") are declared with class ViewClass extends Marionette.ItemView
@type: "whatever" This means you can't use
I on the other hand must confess I've never understood the need for/use of a context-per-view approach. What's to be gained?
You don't need a context-per-view for that. That's what the DI is for. On the contrary, having lesser contexts is easier since you don't have to deal with setting up any kind of context hierarchy (or far less). Also, IMO a view hierarchy should be exactly that: a view hierarchy. Nothing else has no business with how views relate to each other and whether a view has sub-views or not. Even the subviews themselves should have no knowledge whatsoever what their place in the hierachy is, which makes hierarchy changes lightning fast.
Seriously, you don't need multiple contexts nor //file: AppContext
this.wireValue('context', this);
this.wireView('FooView', FooView); //file: FooView
Marionette.ItemView.extend({
wiring : ['context'],
initialize : function(){
console.log("context received:", "undefined" !== typeof this.context);
}
});
On the contrary. Since they're this and that-agnostic I can delay the high-level decisions until they're needed. I always start out with a single context (and bundle) and then when the need arises I start splitting it up. Since my components are totally agnostic it's very trivial to do so.
Of course. And I just love discussing stuff like this. |
As a final note: I'm convinced now we should create a |
Here I am opening my eyes at 3:30 AM : "oh... I think I understand what he meant by "static"... |
😆 |
As reported by @mmikeyy in #72
and
The text was updated successfully, but these errors were encountered: