Force child states to parse on initial data

[bobholt]

fixes #146

I ran into this issue today, and would like to restart the conversation.

This fixes the failing test @HenrikJoreteg created, but some open questions remain:

• should this be the default behavior for both children and collections?
• if not, does it make sense to add an option somewhere?

[pgilad]

I think parsing collections/children is dangerous, since some people use enveloping (that's what parse is for). This might cause nested objects to be mis-parsed.

[bobholt]

I don't disagree @pgilad, especially when it comes to collections, but wanted to get the discussion restarted with a PR with passing tests.

Would it make sense to have an option on ampersand-state to set whether or not to parse children/collections? Something like this?

var MyState = AmpersandState.extend({}, {parseChildren: true, parseCollections: false});

For my use-case, I'm parsing json-api formatted data where all of my state/model data comes in the following format:

{
  data: [
    {
      "id": "1",
      "attributes": {
        "name": "Bob Holt",
        "rank": "pvt",
        "serial": 12345
      }
    },
    {
      "id": "2",
      "attributes": {
        "name": "Jean Valjean",
        "rank": "pvt",
        "serial": 24601
      }
    }
  ]
}

I only want the collection to parse once (to remove the data envelope), but when I combine related models into a nested structure, the models all still need to be parsed in the same way.

The discrepancy is that in this setup:

var Employee = AmpersandState.extend({
  props: {...},
  collections: {
    teams: Teams
  },
  children: {
    position: Position
  }
});

the individual models in the Teams collection trigger parse, but the position does not. I would expect at least all models to behave in the same way.

[pgilad]

I think parse should almost never be (automatically) triggered for nested children/collections.

It could be set manually (i.e parseChildren or parseCollection via options) but not automatically.
Parse should only be triggered automatically for parent that runs an ajax request and receives a response from the server, and even in this case, there is no need to parse for children:

{
  data: [
    {
      "id": "1",
      "attributes": {
        "name": "Bob Holt",
        "rank": "pvt",
        "serial": 12345
      }
    },
    {
      "id": "2",
      "attributes": {
        "name": "Jean Valjean",
        "rank": "pvt",
        "serial": 24601
      }
    }
  ]
}

Even in your example, if attributes is a child, it doesn't need to be parsed. No server that I know of serves data this way:

{
  data: [
    {
      "id": "1",
      "attributes": {
          "data: {
             "name": "Bob Holt",
             "rank": "pvt",
             "serial": 12345
          }
      }
    },
   ]
}

[bobholt]

Thanks, @pgilad. I'll update the PR later today with these as optional.

You're right, no server would serve data that way, but I think you're mixing up the parsing from the collection and from the model and what happens when models that need parsing are nested.

The collection's parse just removes the data property and returns the array. This only happens once when data is loaded, and is the collection's parse method:

parse: function(response) {
  return response.data;
}

Cool.

But all models need to be flattened every time:

{
  "id": "1,
  "attributes": {
    "name": "Bob Holt",
    "rank": "pvt",
    "serial": 12345
  }
}

needs to be:

{
  "id": "1",
  "name": "Bob Holt",
  "rank": "pvt",
  "serial": 12345
}

The nested models in the response retain the attributes property, which still needs to be parsed out when adding as children:

// after collection parsing to nest relations
{
  "id": "1",
  "attributes": {
    "name": "Bob Holt",
    "rank": "pvt",
    "serial": 12345,
    "bestPal": {
      "id": "2",
      "attributes": {
        "name": "Jean Valjean",
        "rank": "pvt",
        "serial": 24601
      }
    },
    "notBestFriends": [
      {
        "id": "3",
        "attributes": {
          "name": "Jean Luc Picard",
          "rank": "capt",
          "serial": 1701
        }
      },
      {
        "id": "4",
        "attributes": {
          "name": "Han Solo",
          "rank": "cpt",
          "serial": 1138
        }
      }
    ]
  }
}

In current ampersand-state, the models in notBestFriends are parsed automatically (in my case, removing the attributes key), but the model in bestPal is not. That's inconsistent. The parse method of the collection notBestFriends doesn't run, and I'm happy with that (nobody would add a data property in there).

This is a reiteration of the problem described and captured in a full test-case in #146.

[latentflip]

Hey @bobholt, so I'm totally on board with getting this fixed, but I think @HenrikJoreteg's original tests are lacking somewhat, such that while this PR will pass the tests, it's not actually going to work in practice.

The real question isn't really is parse called at all, but, is parse called at the time when we set the data we want to be parsed. Really the tests should be testing that parse received the data sent to add (e.g. for child { name: 'mary' }). (I appreciate that's not your mistake :).

Currently in this PR, we're just going to call parse when we initialize the empty objects, which isn't going to do a whole lot.

We're going to need to parse on set for the child data, which occurs here: https://github.com/AmpersandJS/ampersand-state/blob/master/ampersand-state.js#L165-L167 as called in the constructor here: https://github.com/AmpersandJS/ampersand-state/blob/master/ampersand-state.js#L43

Which ultimately starts to look a lot like: #109 and I believe if that was merged it would fix this issue too, but as per the discussion there appears to be a bug with double parsing in #109 :/

[lukekarrys]

I also ran into a slightly related issue with #90, where if you want to get events (such as add) when setting the initial child collections, the parent passes {silent: true} so those events are never triggered.

Just adding this as additional information in case someone runs into that same thing, not saying that it needs to be fixed as part of this PR.

[pgilad]

Please see my response on #146
Apparently that original intent was a mis-understanding of what parse should do (unenvelope and not manipulate).
Original author seemed to agree.

That said - I'm against parsing as default when initializing non-server data.