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

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

Open
drewfish opened this Issue Jul 10, 2012 · 27 comments

Comments

Projects
None yet
7 participants
Contributor

drewfish commented Jul 10, 2012

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.)

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? :)

focuzz commented Jul 14, 2012

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...

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:
yahoo#259 (comment)

focuzz commented Jul 16, 2012

Hope this helps yahoo#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, [], { }, { });

focuzz commented Jul 18, 2012

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 Jul 19, 2012

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.

  • It mixes data and meta information into the same namespace (it uses attributes for all).
  • isNew is calculated based on a false assumption. With data moved apart from meta information, isNew and isModified can be attributes, as they should
  • It does not support multiple field primary keys, which is a normal practice in most existing databases.
  • method toJSON confuses two separate features that should have been split into different methods.
  • the mix of data and metadata into the same space makes it difficult to add extensions because you risk adding attributes which might collide with existing database field names or, alternatively, it forces you to add a layer of translation to data coming from existing data sources to rename conflicting names. This is a never-ending situation because you never know which future feature might add a new attribute name and, in the future, create a conflict you didn't plan for.

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 Jul 19, 2012

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

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.

Collaborator

ericf commented Jul 19, 2012

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.

  • A coalesced change event which fires anytime one more attribute values change has proven to be an extremely useful feature of Model. In fact most places where Attribute is used, like in Widget, this feature is also very desirable. Because it has proven so useful, this feature is moving into Attribute. This feature was not added to Attribute to begin with because of resourcing issues, and the worry of regressions in Attribute since it's depended on so heavily within YUI.
  • Attribute aliases are the other reason Model overrides Attribute methods. We wanted to provide a consistent, always present, id attribute on all models, independent of whether the model used one of its other attributes to actually represent the model's id; e.g. a Model which uses uuid. This required id to also work an a alias for another attribute. The benefits are the consistent API and slight level or indirection for things consuming models (like views). @sdesai has provided an alternate implementation of attribute aliases which works solely on getters and setters.

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.

Contributor

rwaldura commented Jul 19, 2012

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

Satyam commented Jul 19, 2012

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.

Collaborator

caridy commented Jul 19, 2012

@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!

Collaborator

caridy commented Jul 19, 2012

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, [], { }, { });

Contributor

drewfish commented Jul 19, 2012

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.

Collaborator

caridy commented Jul 19, 2012

@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!

Collaborator

ericf commented Jul 19, 2012

@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.

Contributor

drewfish commented Jul 19, 2012

@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.

Collaborator

caridy commented Jul 19, 2012

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.

Contributor

drewfish commented Aug 1, 2012

@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 Aug 1, 2012

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.

Contributor

drewfish commented Aug 1, 2012

@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 Aug 1, 2012

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.

Contributor

drewfish commented Aug 1, 2012

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.

focuzz commented Aug 1, 2012

@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 Aug 2, 2012

@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.

Is the whole YAF a dead effort?

Collaborator

caridy commented Oct 25, 2012

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment