YahooArchive / mojito

[archiving soon] Yahoo! Mojito Framework
BSD 3-Clause "New" or "Revised" License
1.57k stars 214 forks source link

[v1 B-55829] Mojito "model" should converge to Y.Model #259

Open drewfish opened 12 years ago

drewfish commented 12 years ago

Mojito's idea of a model should align closely with YAF's idea of a model . This might even go so far as to throw away Mojito's models and use Y.Model instead.

(The goal is that an app built using YAF can migrate easily/naturally into a mojito app.)

trungpham commented 12 years ago

Yay. I never understood the idea of the current Mojito Model.

It cannot be instantiated. It does not hold any data. It looks like a group of static functions? Why was it called a model to begin with? :)

focuzz8 commented 12 years ago

As I see, models as they exist, can be usefulfor example as gateways for external api (xml or any other - not just rest) etc. So they are useful, but are they "data models", probably "service models"? Just my thought...

trungpham commented 12 years ago

Exactly. They are just service model. They hold no data.

The trick here to somehow migrate the data from the node to the browser after node populated the data. On Jul 14, 2012 2:40 PM, "Alexey Shafranovich" < reply@reply.github.com> wrote:

As I see, models as they exist, can be useful as for example as gateways for external api (xml or any other - not just rest) etc. So they are useful, but are they "data models", probably "service models"? Just my thought...


Reply to this email directly or view it on GitHub: https://github.com/yahoo/mojito/issues/259#issuecomment-6986884

focuzz8 commented 12 years ago

Hope this helps https://github.com/yahoo/mojito/pull/269 This just enables Y.Model to be passed into ac.models[NAME] this way: Y.mojito.models[NAME] = Y.Base.create('pieModel', Y.Model, [], { }, { });

rwaldura commented 12 years ago

See also forum discussion at http://developer.yahoo.com/forum/Yahoo-Mojito/Integrating-with-YUI-3-5-MVC-App/1339551100881-219004e1-417c-4bd9-bf46-4c5d7ee7c1cc

focuzz8 commented 12 years ago

Well, I can say that transparent routing support on server and client can be right thing. It can be implemented manually using Y.Router (with manual client-side route-to-action mapping) but it can be perfect to share routes.json config on client and server, but it requires to spend some time to be done. About dynamic mojit loading - I've created "placeholder" - generic mojit based on code of LazyLoad, that reacts on messages(events) addressed to him, and reloads it's content (recreates inner mojit). Can share some thought about it. And I think, good idea is to give ability (I don't tried yet) for binders, to access models. It can be useful for form validation for example. I think there is no need of Y.View on client - controller already does everything. Just my thought and my vision.

Satyam commented 12 years ago

Please, don't add Y.Model to Mojito as it is. It has several architectural shortcomings and it would be a bad thing if those propagated to Mojito.

For more information see:

http://satyam.github.com/WhyGalleryModel.html (just read a few lines after each of the headings to get an idea of the issues) http://yuilibrary.com/gallery/show/md-model http://yuilibrary.com/forum/viewtopic.php?p=32738#p32738

Satyam commented 12 years ago

As a proof of the bad design of Y.Model allow me to point to the following methods of Y.Model itself, which are originally defined in Attribute (or one of its constituents) and had to be redefined in Model as a patch to compensate for its bad use of those attributes:

Some of them are declared in the API docs (click on the 'protected' checkbox to see them) as "Duckpunches", whatever that might mean (non-native English speakers like myself would certainly appreciate if documenters avoided some halfassed colloquialisms in formal documents). To me, it means "unnecessary overhead to compensate for a bad design descision".

Thus, Y.Model is already patching itself for the improper use of a core YUI module.

ericf commented 12 years ago

First I'd like to respond to the statement that proof that Y.Model is a bad design can be determined by the fact that it's overriding Y.Attribute methods, without understand why it's overriding those methods.

As a proof of the bad design of Y.Model allow me to point to the following methods of Y.Model itself, which are originally defined in Attribute (or one of its constituents) and had to be redefined in Model as a patch to compensate for its bad use of those attributes:

  • get
  • set
  • setAttrs
  • _defAttrChangeFn
  • addAttrs

The reasons Model overrides these Attribute methods are to provide features that did not exist in Attribute, but which are planned to be added to it.

Please understand that we were not happy to have to override Attribute to gain these features. But we felt there were important enough and, all things considered, felt this was the best implementation of these feature at that time. Luckily, we have a path moving forward which would alleviate the need for Y.Model to override Y.Attribute, and this is something we are working on.

rwaldura commented 12 years ago

Oh interesting. Wasn't aware of this discussion, thanks!

Satyam commented 12 years ago

All the attribute issues would have been avoided if data and meta data had been split from the start. If you look at my gallery-md-model, it has a 'coalesced' data event from the start without any patching. I would rather not know the reasons to patch Model, I would rather have a better Model that needs no patching.

So far in all the discussions I've been involved in, nobody has ever pointed out a single issue with my proposed solution. I am sure it has some issues but my point is: it seems nobody has ever taken the time to look at it!!!!!! I could have filled the whole thing with plain trash and it would have been the same since nobody would have noticed. Nobody has ever said that my model doesn't work because of this and that simply because nobody cared to look. Hopefully, it doesn't have any issue, but all the replies I've ever got are in the lines of "because the says so"

All the reasons I hear about the decisions taken seem to go around technicalities on how to use the YUI library for the sake of the YUI library itself and nice examples with data read via YQL or some such. Someone, long ago, asked for a real CRUD application with a real SQL database. Can someone please remember that a Model is meant to represent real data, not about cool programming?

If you cannot manipulate a real, existing SAP table, you don't have a good enough model. And you cannot do it with plenty of patching. My intention is for Model to do it without any patching, and those include the patches that Model itself already has. None of my suggestions are incompatible with YQL datatables or other non-SQL type of data. My solution does not exclude your datasource, Y.Model excludes plenty of regular, existing SQL tables.

caridy commented 12 years ago

@ericf @Satyam, let's take some time to evaluate this, and come up with a plan that fits mojito and yui. For us, It is a good opportunity to revisit the use case of model now that we are doing some refactor in Axis to use Y.Model without the rest of the Y.App framework in the context of Mojito. We will see!

caridy commented 12 years ago

Btw, I don't think this discussion affects the pull request in the first place. We still need to support Y.Model instances in mojito, and @focuzz proposal seems to be a good starting point. e.g.:

Y.mojito.models[NAME] = Y.Base.create('pieModel', Y.Model, [], { }, { });

drewfish commented 12 years ago

I think what @caridy is saying (please correct me if wrong) is that using a Y.Model for a Mojito Model is optional. You can still use a normal Mojito Model, or other thing, if you want.

caridy commented 12 years ago

@drewfish yes. for backward compatibility, and for simple use cases, a current mojito model can work just fine. I don't see why we should remove it. And I think the commit from @focuzz fulfills that!

ericf commented 12 years ago

@drewfish @caridy If both end up existing in Mojito, at least in documentation, should what's currently called "Model" in Mojito be referred to as a "data service"?

It seems that some of the confusion around Mojito models is that they more closely match people conceptions of a static data service. Whereas when people's conception of a model is something instance-based which encapsulates data with the methods to interactive with and transmit that data.

drewfish commented 12 years ago

@ericf Well, the trick about Mojito Models is that Mojito actually gives no prescription on the structure or semantics of a Model. They don't even have to have methods! So to say that "Mojito Models are data services" is inaccurate. Mojito Models aren't anything, just a place in the Mojito framework for a user to put functionality, and we suggest that they put "model" functionality (whatever that means) there.

caridy commented 12 years ago

I have mixed feelings about this. I think once Y.Model lands in mojito we can definitely know how to call them, and to have some documentation explaining the different type of models or data-services or whatever, and how they will work with mojito.

drewfish commented 12 years ago

@caridy That's fine. I agree that we likely want to update our documentation to primarily discussion Y.Model.

(The fact that other types of code/objects can be used is really an advanced feature, specifically so that Mojito has an out to deal with concerns like @Satyam raises.)

Satyam commented 12 years ago

Have I mentioned performance? Y.Model is based on Backbone.Model. Both have a get method.

This is the original 'get' method of Backbone: https://github.com/documentcloud/backbone/blob/master/backbone.js#L236

This is my getValue method, which is the equivalent but does not use the get method inherited from Attribute: https://github.com/Satyam/yui3-gallery/blob/master/src/gallery-md-model/js/gallery-md-model.js#L155

This is what Y.Model ends up using by having to use Attribute (the public get() method calls _getAttr() ):

http://yuilibrary.com/yui/docs/api/files/attribute_js_AttributeCore.js.html#l468

Sorry about all of those (apparently) zillions of Backbone developers who would never switch to YUI because they are completely unable to rewrite all their model.get() and model.set() to use getValue() and setValue() but preserving the name of the public interface for all those poor souls comes at a great cost.

I just mentioned get and its equivalents because they are brief enough to see in a screenfull. Have you checked the set/setValue on each?

Try running some performance tests.

Y.Model might be based on the very popular Backbone, but it has not improved on it. Not a bit.

drewfish commented 12 years ago

@Satyam You've clearly communicated your reservations about Y.Model. We've heard you. In Mojito, you can use Y.Model if you want, or use something else if you want.

This issue is probably poorly named. It should instead say "Mojito model should allow use of Y.Model".

Satyam commented 12 years ago

Whatever you show in the examples and tutorials is what developers will end up using. Implicitly and in the minds of most people learning Mojito, you would be endorsing Y.Model. It really doesn't matter how you phrase your disclaimer. Lots of people will waste a lot of time based on that. So be it.

drewfish commented 12 years ago

Yes, we do plan on endorsing Y.Model. It does work for many people. If anyone wants to use something else that is possible too.

focuzz8 commented 12 years ago

@Satyam @drewfish It seem like everybody can hack a little bit to get whatever he wants. I think it's not a problem to make some different samples with current (plain) model, with Y.Model and with custom model. For me for example problem with keys is not a problem because

  1. i'm using guid as pk in most cases (not a composite key)
  2. most valuable for me - i do not expose db objects directly to frontent - it's more secure and it's more productive because frontend gets data in way it processes it but I understand that your usecases (to @Satyam) exist, and I think they should be documented for example in model or extend mojito section. What do you think?
Satyam commented 12 years ago

@focuzz I don't want Mojito samples with my custom Model. My goal was to show the design errors in Y.Model and have those fixed in Y.Model itself and, to do that, I showed how it could be done better, perhaps not right, as I cannot claim to have the perfect solution, but at least much better than what's on offer.

BTW, I wouldn't mind if someone showed me what's wrong with my design or that what I call defects in Y.Model are not such or that the performance of Y.Model is not much worst than mine.

I have not blindly criticized Y.Model, I have spent quite a lot of time and effort studying it and finding solutions which I took more time and effort to show how to fix. I have not seen any indication that anyone has ever bothered to take a look at what I coded, documented and explained.

It is not that one or the other model can be used depending on use cases, Y.Model has design errors. Those errors might not be evident in some use cases, but they are still there. It is not mainly about whether you use multiple primary keys, the core issue is not there, multi-field primary keys might be a nice feature, but it is not critical. The core issue here is that data and meta-data should be separated. You can't use attributes to hold them both. You have to leave Attribute's set and get alone and use setValue and getValue (or any other name you want) for data. Just look at the links I provided a few posts before. See how my getValue matches more closely Backbone.Model.get intent if not its name. The primary design goal for Y.Model seems to have been to make its interface identical to that of Backbone.Model at whatever cost. The cost has been too high.

@drewfish It makes sense that you endorse Y.Model. It wouldn't hurt if you fix it first. It is not a matter of using one or the other. One is a badly copied version of Backbone.Model plus a whole bunch of patches to a core component (which should not be patched) to enforce that arbitrary decision to make Y.Model look like Backbone, as if anyone could prize that so badly that they would be willing to live with the restrictions, the patches and their performance hit.

By all means, do use Y.Model, but fix it first.

trungpham commented 12 years ago

Is the whole YAF a dead effort?

caridy commented 12 years ago

@trungpham it is not dead, we have people actively working on this, but it is not moving as fast as we want, mostly due to the other initiative to stabilize performance, and that one has a higher priority at the moment. In any case, here is the trello board for this exercise in case you want to follow-up/subscribe:

https://trello.com/board/mojito-yaf/50627b0ad717546b7685105d