Skip to content

Instantly share code, notes, and snippets.

@aaronj1335
Last active December 15, 2015 10:29
Show Gist options
  • Select an option

  • Save aaronj1335/5245990 to your computer and use it in GitHub Desktop.

Select an option

Save aaronj1335/5245990 to your computer and use it in GitHub Desktop.

we should probably get some unit tests around the following corner cases:

  1. collection load's

  2. a collection has been loaded and includes a particular model

  3. that model is updated server-side, not on the page

  4. the collection is re-loaded, and the change from the server side is applied to the local copy of the model

  5. i don't know if the model triggers a change event in this case

  6. pertinent code: when a query resolves and converts the plain objects into model instances

  7. model creation (i.e. actually saving a new model, not just instantiating it):

  8. when a model is .save()'ed and it results in a create request, it should trigger some event on the manager, probably in the associate method. let's call these events from the manager 'add' events for clarity.

  9. if a collection is just loading everything, i.o.w. has no query, limit, or offset, then it should probably listen for these events, Collection#add() the new model to its set, and trigger an 'update'

  10. if a collection does have some sort of query, it can't be sure whether or not it should include the new model, so should it call .refresh()? this may be too aggressive

@ralphsmith80
Copy link
Copy Markdown

To clarify I think what you mean by;

  • when a model is .save()'ed, it should trigger some event on the manager, probably in the associate method

is

  • when save() is called on a model that results in a create request, it should trigger some event on the manager, probably in the associate method

The reason being that Manager::associate is only called in that case or during Model::init.

@aaronj1335
Copy link
Copy Markdown
Author

@ralphsmith80 good catch, i'll update the original file.

@ralphsmith80
Copy link
Copy Markdown

@aaronj1335

Based on that update I'm not sure that these should be considered separately;

  • if a collection is just loading everything, i.o.w. has no query, limit, or offset, then it should probably listen for these events and trigger an 'update'
  • if a collection does have some sort of query, it can't be sure whether or not it should include the new model, so should it call .refresh()? this may be too aggressive

When the new model is created it's not added to the collection. It's only added to the managers list of models. So we can handle both cases by calling load which holds the existing query if one is present and triggers an update.

As you mentioned above this maybe to aggressive, but if we trigger an update without a refresh the new model won't be in the collection anyway. The only alternative I see is to call Collection::add each time the manager triggers the 'some new event'.

Am I missing something?

EDIT:
I don't really see any case where we would want notify the manager other than in Model::save when creating. If we try to do this in Manager::associate were going to have to determine if the call was triggered from a collection load which would cause problems.

I don't expect you to remember all this code just throwing down what I come across while trying to get to a working solution.

@aaronj1335
Copy link
Copy Markdown
Author

@ralphsmith80 i updated the cases w/ numbers so we could reference them more clearly. i also gave the event in scenario 2.1. the name 'add' -- though it doesn't necessarily need to be this name, really it's just the event that happens when a new model is persisted server-side, and it comes from the manager.

so yeah, you're not missing something. one thing i omitted, in the 2.2. scenario the collection should listen for these events, when it gets one it should Collection#add() then new model to its set, and then trigger an update.

like u said, this is an optimization, because every collection could could just call this.refresh() on every manager 'add' event, but i think that'd be too many requests. i don't think it's a premature optimization, but that's just a judgement call.

@aaronj1335
Copy link
Copy Markdown
Author

i'll add that clarification to 2.2.

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