Multiplexed data source #664

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@tim-evans
Member

tim-evans commented Dec 21, 2011

I'm proposing an addition to the provided SC.DataSources that allows for the multiplexing / demultiplexing of data from one parent data source to specific child data sources. This looks similar to SC.CascadeDataSource, but in the end allows for data sources to be designed in trees rather than cascades, with a link back to it's parent data source.

SC.MultiplexedDataSource is designed to be used by a single stream / source of data that has lots of different / complex record types that would require spaghetti code like so:

createRecord: function (store, storeKey) {
  switch(recordType) {
  case MyApp.ChatMessage:
    // dispatch to chat message record creator
  case MyApp.Buddy:
    // dispatch to buddy record creator
  case MyApp.VCard:
    // dispatch to vCard record creator
  default:
    return NO;
  }
}

Instead, each record can be handled by a stand-alone data source, which can be a SC.DataSource, SC.CascadeDataSource, or SC.MultiplexedDataSource. It optionally requires the helper mixin SC.DataSourceDelegate which will help with accessing parent data source(s) by overriding unknownProperty.

This is designed to ease unit testing and reduce spaghetti / boiler plate in applications / frameworks that require a non-trivial data source.

A simple, non-trivial, working example of a multiplexed data source can be found at https://gist.github.com/1507264 (the cascade version can be found at https://gist.github.com/1510948).

added MultiplexedDataSource for splitting data sources by record type
this is intended to create modular, pluggable data sources
@sevifives

This comment has been minimized.

Show comment Hide comment
@sevifives

sevifives Apr 16, 2012

Contributor

I like the idea of this, but for some reason, the idea of it down in the store level seems weird.

It feels like we should be able to pull this stuff up into the record level instead of creating delegates and adding on more store code.

That way you could just specify the necessary CRUD operations on your base record class instead of in a delegate method on the store.
I don't believe they should be separated and might lead to some confusion.

Contributor

sevifives commented Apr 16, 2012

I like the idea of this, but for some reason, the idea of it down in the store level seems weird.

It feels like we should be able to pull this stuff up into the record level instead of creating delegates and adding on more store code.

That way you could just specify the necessary CRUD operations on your base record class instead of in a delegate method on the store.
I don't believe they should be separated and might lead to some confusion.

@tim-evans

This comment has been minimized.

Show comment Hide comment
@tim-evans

tim-evans Apr 16, 2012

Member

@sevifives I agree, but that requires a larger API change to records than the scope of this. Also, are you meaning that the CRUD operations should exist on the instance or class of the SC.Record? (The data sharing aspect of this model is the little added bonus that makes this very useful IMO)

Member

tim-evans commented Apr 16, 2012

@sevifives I agree, but that requires a larger API change to records than the scope of this. Also, are you meaning that the CRUD operations should exist on the instance or class of the SC.Record? (The data sharing aspect of this model is the little added bonus that makes this very useful IMO)

@sevifives

This comment has been minimized.

Show comment Hide comment
@sevifives

sevifives Apr 16, 2012

Contributor

I would thing the CRUD overrides would work like this:

App.YourRecord = SC.Record.extend({
createRecord: function () {},
deleteRecord: function () {},
updateRecord: function () {},
readRecord: function () {}
});

App.OtherRecord = App.YourRecord.extend({
createRecord: function () {/* would override YourRecord, but extends the rest */}
});

Activerecord/datamapper is what comes to mind: you can override all the necessary stuff in the model definition, basically.

Contributor

sevifives commented Apr 16, 2012

I would thing the CRUD overrides would work like this:

App.YourRecord = SC.Record.extend({
createRecord: function () {},
deleteRecord: function () {},
updateRecord: function () {},
readRecord: function () {}
});

App.OtherRecord = App.YourRecord.extend({
createRecord: function () {/* would override YourRecord, but extends the rest */}
});

Activerecord/datamapper is what comes to mind: you can override all the necessary stuff in the model definition, basically.

@tim-evans

This comment has been minimized.

Show comment Hide comment
@tim-evans

tim-evans Apr 17, 2012

Member

+1 That seems completely reasonable to me. One thing that I'd like answered before I go and do anything about this would be what would this be for each of these functions?

Member

tim-evans commented Apr 17, 2012

+1 That seems completely reasonable to me. One thing that I'd like answered before I go and do anything about this would be what would this be for each of these functions?

@sevifives

This comment has been minimized.

Show comment Hide comment
@sevifives

sevifives Apr 17, 2012

Contributor

I'm inclined to think this should be the instance. I thought there might be a caveat with Create, but it's created locally first and the store and storeKey would be passed in, so I don't believe that'd be an issue. Thoughts?

Contributor

sevifives commented Apr 17, 2012

I'm inclined to think this should be the instance. I thought there might be a caveat with Create, but it's created locally first and the store and storeKey would be passed in, so I don't believe that'd be an issue. Thoughts?

@tim-evans

This comment has been minimized.

Show comment Hide comment
@tim-evans

tim-evans Apr 17, 2012

Member

There's a wrinkle in doing so, since records aren't actually materialized until after the run loop finishes for laziness purposes. I'm not sure how much this will affect performance though.

Member

tim-evans commented Apr 17, 2012

There's a wrinkle in doing so, since records aren't actually materialized until after the run loop finishes for laziness purposes. I'm not sure how much this will affect performance though.

@publickeating

This comment has been minimized.

Show comment Hide comment
@publickeating

publickeating May 29, 2012

Member

I've visited this one a few times and each time I feel reluctant to merge it in. Of course the code is excellent and the functionality is great, but I've never been able to shake the feeling that this is too high end specific to be a default piece of the framework. My feeling is that the cost of the extra code and of the extra confusion between Cascade and Multiplexed is slightly higher than the value of it being built in.

I really appreciate the lengths you've gone to to complete this, but how do you feel about keeping it as an optional include somewhere? Feel free to disagree, but in any case I think the options are:

  1. It should be in the framework, put it in sproutcore/frameworks/datastore
  2. It should be in the framework optionally, rename sproutcore/frameworks/experimental to sproutcore/frameworks/extensions and put it there
  3. Create a new repo for extensions and place it there. Also move the code from sproutcore/frameworks/experimental into the extensions repo and remove the experimental sub-framework from sproutcore.

I'm leaning towards 3 right now, because that gives the extensions their own README and a bit more visibility to what they provide. It also lets you act as a project owner of your extension and take it in your own direction more freely without me breathing down your neck like this ;-)

Sorry for not being more compromising, please feel free to disagree.

Thanks,

ps., As you can tell, I don't like having code labeled experimental in the framework, it implies that it shouldn't be used. If it is so rough that it shouldn't be used, then it shouldn't be in there at all.

Member

publickeating commented May 29, 2012

I've visited this one a few times and each time I feel reluctant to merge it in. Of course the code is excellent and the functionality is great, but I've never been able to shake the feeling that this is too high end specific to be a default piece of the framework. My feeling is that the cost of the extra code and of the extra confusion between Cascade and Multiplexed is slightly higher than the value of it being built in.

I really appreciate the lengths you've gone to to complete this, but how do you feel about keeping it as an optional include somewhere? Feel free to disagree, but in any case I think the options are:

  1. It should be in the framework, put it in sproutcore/frameworks/datastore
  2. It should be in the framework optionally, rename sproutcore/frameworks/experimental to sproutcore/frameworks/extensions and put it there
  3. Create a new repo for extensions and place it there. Also move the code from sproutcore/frameworks/experimental into the extensions repo and remove the experimental sub-framework from sproutcore.

I'm leaning towards 3 right now, because that gives the extensions their own README and a bit more visibility to what they provide. It also lets you act as a project owner of your extension and take it in your own direction more freely without me breathing down your neck like this ;-)

Sorry for not being more compromising, please feel free to disagree.

Thanks,

ps., As you can tell, I don't like having code labeled experimental in the framework, it implies that it shouldn't be used. If it is so rough that it shouldn't be used, then it shouldn't be in there at all.

@tim-evans

This comment has been minimized.

Show comment Hide comment
@tim-evans

tim-evans May 29, 2012

Member

@publickeating I actually tend to agree with you. This was something that we use internally for our app, but in the end is not essential to SproutCore and possibly confuses the already confusing data layer for new developers.

+1 to adding an extensions repo, it's more fitting for this type of PR.

Member

tim-evans commented May 29, 2012

@publickeating I actually tend to agree with you. This was something that we use internally for our app, but in the end is not essential to SproutCore and possibly confuses the already confusing data layer for new developers.

+1 to adding an extensions repo, it's more fitting for this type of PR.

@publickeating

This comment has been minimized.

Show comment Hide comment
@publickeating

publickeating May 29, 2012

Member

That's good to read, I will make such a repo soon.

On a related note, once it's posted, it should be highlighted on the blog and if you want to write a short post about it, that'd be great. Otherwise, I will still make a small mention about it there to give it some extra visibility for anyone searching for that type of functionality.

Member

publickeating commented May 29, 2012

That's good to read, I will make such a repo soon.

On a related note, once it's posted, it should be highlighted on the blog and if you want to write a short post about it, that'd be great. Otherwise, I will still make a small mention about it there to give it some extra visibility for anyone searching for that type of functionality.

@publickeating

This comment has been minimized.

Show comment Hide comment
@publickeating

publickeating Jun 14, 2012

Member

I'm still going back and forth on the utility of an 'extensions' repo. But we can at least try it out for now. I copied your files and updated the README on https://github.com/sproutcore/extensions

I had trouble getting the tests to run though, but never figured it out.

Thanks for your patience.

Member

publickeating commented Jun 14, 2012

I'm still going back and forth on the utility of an 'extensions' repo. But we can at least try it out for now. I copied your files and updated the README on https://github.com/sproutcore/extensions

I had trouble getting the tests to run though, but never figured it out.

Thanks for your patience.

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