Initial functional commit #1
Conversation
| @@ -1,2 +1,44 @@ | |||
| # backbone-meteor | |||
| Backbone.Model and Backbone.Collection that supports observing a publication-client based reactive query | |||
| # backbone-publication | |||
There was a problem hiding this comment.
Let's make this public btw. We initially promised to a year and a half ago haha https://mixmax.com/blog/meteor-and-backbone
README.md
Outdated
| // During the bootstrapping process we normally set most collections/models - | ||
| // using `backbone-publication` collections/models is no different. | ||
|
|
||
| setUserFeatures(new FeatureCollection(initialPayload.features, { |
There was a problem hiding this comment.
To make this public I'd remove the setUserFeatures part. You can keep "bootstrapping" though, just s/we normally set/we normally initialize.
| Where FeatureCollection is defined as: | ||
|
|
||
| ```js | ||
| import { PublicationCollection } from 'backbone-publication'; |
There was a problem hiding this comment.
This is a nice design, I had been picturing continuing to get the classes like Backbone.PublicationCollection but I like this better.
README.md
Outdated
| ```js | ||
| import { PublicationCollection } from 'backbone-publication'; | ||
|
|
||
| class FeatureCollection extends PublicationCollection { |
There was a problem hiding this comment.
Unfortunately Backbone doesn't play well with ES6 classes: jashkenas/backbone#3560
There was a problem hiding this comment.
Cleared live, we could make them play nicely enough, but we've decided not to for the moment since it doesn't give us any major gain.
README.md
Outdated
|
|
||
| There really shouldn't be any differences when migrating from | ||
| Backbone.Meteor[Model,Collection] - all the differences are handled under the | ||
| hood. |
There was a problem hiding this comment.
Let's leave this out, it'll become a moot point soon enough :)
src/PublicationModel.js
Outdated
|
|
||
| if (!options.silent) { | ||
| // Trigger events for any nested objects. | ||
| _.each(newAttributes, function(value, key, attributes) { |
There was a problem hiding this comment.
let's use an arrow function
src/PublicationModel.js
Outdated
| if (options.unset) { | ||
| Backbone.Model.prototype.unset.call(this, fullAttributes, options); | ||
| } else { | ||
| Backbone.Model.prototype.set.call(this, fullAttributes, options); |
There was a problem hiding this comment.
PublicationModel.__super__ etc.
src/PublicationModel.js
Outdated
| /** | ||
| * Override unset implementation to use the overridden version of set above. | ||
| */ | ||
| unset(key, value, options) { |
There was a problem hiding this comment.
I know you're just copying this over but I think this will use the overridden version anyway…? http://backbonejs.org/docs/backbone.html#section-75
| /** | ||
| * Sets the reactive query to the given reactive query. | ||
| * NOTE: if you use this functionality, you'll need to also call | ||
| * `startObservingChanges` in order to re-initialize the model's listeners. |
There was a problem hiding this comment.
It sounds fine to me that we would make the user call startObservingChanges again. This should call stopObservingChanges to be safe though, no?
src/index.js
Outdated
| @@ -0,0 +1,8 @@ | |||
| import PublicationCollection from './PublicationCollection'; | |||
| import PublicationModel from './PublicationModel'; | |||
There was a problem hiding this comment.
If you want one liners you can do export { default } from './PublicationModel' or export { default as PublicationModel } from './PublicationModel' btw. And for even shorter, export PublicationModel from './PublicationModel' (the ideal IMO), it looks like you can use this plugin. Background.
|
@wearhere PTAL |
package.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "@mixmaxhq/backbone-publication", | |||
| "name": "backbone-publication", | |||
| "version": "0.0.0", | |||
| "description": "Publication backed Backbone.Meteor and Backbone.Collection classes", | |||
No description provided.