Problem with kb.ViewModel and collection rendering. #34

Closed
xdenser opened this Issue Jul 24, 2012 · 5 comments

Comments

Projects
None yet
2 participants

xdenser commented Jul 24, 2012

Please check following fiddle

http://jsfiddle.net/MygjJ/7/

You will see viewModel is not rendered properly.

I have commented working and not working lines there. In short: problem comes when two books refer same author.
On other side at the end of JS code there is variant that fixes that using

view_model_create

instead of

view_model

in collection options.

So it looks like something wrong with kb,ViewModel.

Owner

kmalakoff commented Jul 25, 2012

@xdenser : thank you for taking the time to put together the jsfiddle and I'm sorry about you encountering this problem.

I've taken a look at the code and see what the problem is. Knockback is resolving view models as it is encountering them so it doesn't know about the view model type of Book Two when it encounters it:

books -> b1 -> a1 -> b1 : OK because it has already cached b1 in the kb.Store
books -> b1 -> a1 -> b2 : not OK because through the relationship a1, it doesn't know the proper type of b2 and creates a generic kb.ViewModel for it.
books -> b2 : not OK because it finds the view model for b2 which was created of the wrong type through a1

I think I might need to update the code to do something like adding type frames:

When books creates its store, it pushes its create information like:
store.creators = {Book: {BookCollection: bookViewModel}}

Then when Book1 is created, it updates the tree like:
store.creators = {Book: {BookCollection: bookViewModel}, Author: {Book: kb.ViewModel}}

Then when Author is created, it updates the tree like:

store.creators = {Book: {BookCollection: bookViewModel, Author: bookViewModel}, Author: {Book: kb.ViewModel}}

So the types are properly propagated by looking up your type, your parent's type and finding the correct view model, and if there is not one specified, kb.ViewModel looks up its tree to use the same type.

Alternatively, I could make everything really explicit like in Backbone.Relational:

   Book = Backbone.RelationalModel.extend({
       defaults:{
        name: 'untitled'
       },
       idAttribute: "_id"
    }),
    Author = Backbone.RelationalModel.extend({
       defaults:{
         name: 'untitled'
        },
        idAttribute: "_id",
        relations:[
            {
            type: 'HasMany',
             key: 'books',
             relatedModel: Book,
             includeInJSON: "_id",
             reverseRelation: {
             key: 'author',
             includeInJSON: "_id"
        }}
        ]
    }),
    BookStore = Backbone.RelationalModel.extend({
       relations:[
      {
          type: 'HasMany',
          key: 'books',
          relatedModel: Book
      },
      {
          type: 'HasMany',
          key: 'authors',
          relatedModel: Author
      }
      ]
      view_models: {books: bookViewModel}
    });

Any preferences or thoughts?

Kevin

xdenser commented Jul 26, 2012

thank you for response

I do not understand fully what do you mean when saying "make everything really explicit". I think you should not try to pickup ViewModels when they are not explicitly declared. So when I say use that ViewModel for collection you should not use it for other deeper attributes automatically, or add an option for that.

Owner

kmalakoff commented Jul 26, 2012

By making things really explicit, I mean explicitly declaring the view model type like in the second proposal:

Author = Backbone.RelationalModel.extend({
    ...
      view_models: {books: bookViewModel}

    }),
BookStore = Backbone.RelationalModel.extend({
   ...
   view_models: {books: bookViewModel}
    }),

The thing that doesn't feel right about this approach is making a one-to-one relationship between the Model type and the ViewModel type. This means that if you want to use a different view model for the relationship in a different context (for example, view vs edit), you need to then somehow define a context and look it up like:

Author = Backbone.RelationalModel.extend({
    ...
      view_models: {view: {books: bookViewModel}, edit:: {{books: bookEditViewModel}}}

    }),
BookStore = Backbone.RelationalModel.extend({
   ...
      view_models: {view: {books: bookViewModel}, edit:: {{books: bookEditViewModel}}}
    }),

and somehow pass the context into the create method and propagate it through the store like:

view_model = {   books: kb.collectionObservable(bs.get('books'), {
   view_model: bookViewModel }, context: 'edit') }; 

Another way to explicitly define the view models could be in the collectionObservable creation:

view_model = {   books: kb.collectionObservable(bs.get('books'), {
   view_model: bookViewModel }
   descendants:
      Author: {
         books: bookViewModel }
      }
) }; 

Since the problem is caused by the view model type lookup for nested models which is order-dependent, either the solution will involve: 1) setting up the relationship after the collectionObservable was been created (that technique is currently in the tests), 2) inferring the view model type, 3) finding a way to explicitly map the view models for nested models.

I think maybe making explicit in the collectionObservable create method is best. What do you think?

Owner

kmalakoff commented Aug 12, 2012

@xdenser : sorry for the delay in getting to this....

I have created a fix as part of a new release 0.16.0beta1 in github master.

I tried to keep it simple so this is the proposal:

view_model = {   books: kb.collectionObservable(bs.get('books'), {
    view_model: bookViewModel,
    mappings: {
      'models.author.books.models': bookViewModel
    }
}) };

or

view_model = {   books: kb.collectionObservable(bs.get('books'), {
    mappings: {
      models: bookViewModel,
      'models.author.books.models': bookViewModel
    }
}) };

and if you want to have a create function:

view_model = { books: kb.collectionObservable(bs.get('books'), {
mappings: {
models: { create: function(model, options) {return new bookViewModel(model, options) },
'models.author.books.models': { create: function(model, options) {return new bookViewModel(model, options) }
}
}) };

So basically, in mappings, you supply '.' separated paths with either view model constructors or factory functions. The options hold the factory and path information so they should be propagated.

Would you be willing to take a look at it and tell me what you think? I added your code to https://github.com/kmalakoff/knockback/tree/master/test/issues so you can confirm how it works.

Kevin

Owner

kmalakoff commented Sep 4, 2012

I've released 0.16.0 so closing this.

Please note that 'mappings' was replaced with 'factories' in the final release.

kmalakoff closed this Sep 4, 2012

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