Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2: Collection#parse #3758

Closed
wants to merge 1 commit into from
Closed

Conversation

jridgewell
Copy link
Collaborator

DO NOT MERGE, BREAKING CHANGE This is for the next major, v2.

This is the revert of #3659 (which reverted #3644). So, really, it's just #3644 (which reverted #1407... reverts! 😛).

Why? To mirror Model. Model#set doesn’t parse for you, only instantiation, #fetch, and #save parse.

  • We do it on instantiation because we have to have an instance to call #parse (you can't do it before hand because you don't have an instance)
  • We do it on #fetch and #save because they return data from the server (the reason we have a #parse!)

If you need to parse your Collection's data before #set, #reset, or #add, do it beforehand (you already have an instance):

  • collection.set(collection.parse(models));
  • collection.reset(collection.parse(models));
  • collection.add(collection.parse(models));

Or, you can parse on instantiation if you don't have an instance, just like Model:

  • new Collection(models, { parse: true });

And of course, Collection#fetch parses at the collection level like it always has.


Third times the charm? 😜

…parse-v1-2-1"

This reverts commit 3e334b1, reversing
changes made to ff57b54.
@d-reinhold
Copy link

Line 837 in Collection#set is still checking for options.parse to determine if Model#parse should be called on the attrs, can that be removed as well? Because implementations of Collection#parse should be returning arrays of fully parsed models, right?

@prantlf
Copy link

prantlf commented Jun 15, 2017

@d-reinhold, correction: Implementations of Collection#parse should be returning arrays of objects, which are expected to be consumed by the Model#parse method. If you "massage" the complete response in Collection#parse, what would you have the Model#parse for? How would you parse the response from the REST API returning a single object? Putting all functionality to Collection#parse would limit your framework to the REST API returning collections only..

@prantlf
Copy link

prantlf commented Jun 15, 2017

@jridgewell, your suggestion does not cover the existing functionality.

New: collection.set(collection.parse(models))

As long as collection.parse does not return array of models, but an array of objects, which the models will be constructed with, there is no way to get model.parse called for every particular model. You lose the componentization - way to divide the scope of collection parsing (which is usually simple) and the scope of model parsing (which is usually complicated).

Existing: collection.set(models, {parse: true})

Unlike calling the parse method once from outside, this interface allows to propagate the parsing flag to model constructions. It allows componentization of the parsing code, even in non-homogeneous collections. Such separation encourages code sharing too; collection mixins, for example.

@jgonggrijp
Copy link
Collaborator

I've seen some changing back and forth like this in Underscore as well. I'm not having it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants