parse / toJSON applied inconsistent #111

[MichaReiser]

Fix for the issue parse / toJSON applied inconsistent #111

The solution is more a workaround then a real solution... The code base should be refactored. The local sync should only works with json objects and not with real model instances. This way we could avoid the multiple parsing / serializing of the objects.

The current solution should ensure, that parse and toJSON are applied in a consistent manner. The support for parseBeforeLocalSave has not been extended and has been marked as deprecated. The main reason is, that the localSync should only be a back end that stores the values locally and should behave the exact same way as the remote endpoint. So we should not need an additional method here. The use case mentioned in the other issue is still supported, as you can see in the unit test.

• Override collection.parse to parse the result on the client
• Override toJSON on the collection to be able to write back to the server (not supported by the solution with parseBeforeLocalSave)
• Override parse in the model (if needed). Also override parse in the model.

The implementation ensures, that parse is always passed the JSON representation of the model, independent if the data has been retrieved from the server or from local storage.

Room for improvements

• parse / toJSON is invoked way to often (update: 2x parse, at least once toJSON)
• solution is a workaround
• parseBeforeLocalSave should not be needed
• additional unit tests

The checked in source works for my project I'm working on. To perform the complete refactoring I'm missing the insight and the time...

I hope this solution works for you. If you have any questions or improvements, I will gladly answer them. Micha

How can I assign a pull request to an issue?

[nilbus]

Thanks for implementing this; especially the tests. They will be helpful when eventually we refactor this into a final solution. Because it seems this isn't affecting a lot of our current users, I'm going to hold off on merging this until it has been refactored and is fully ready. I want leave this open though for people who are affected by the same issue. Keep us posted about if you have success or issues with this workaround.

As for the deprecation, it sounds nice in theory to just have dualSync store exactly what the server sends and act no differently. The need for parseBeforeLocalSave as presented in #2 is legitimate though. DualStorage users need a way to parse the server response for a collection fetch. If not, with servers that respond with a collection like in kurtmilam's example, dualStorage will store the entire collection under a single localStorage key as if it were a single model, rather than splitting it up into separate keys for each model like it needs to for consistency when saving and fetching an individual model. We need parseBeforeLocalSave at least for collection fetch and will not deprecate it.

FYI, you can create a pull request over an existing issue by using github's hub command-line tool.

[MichaReiser]

This case should be supported... If you look at the unit test, i have used exactly such a data structure as posted by kurtmilam's. You can override the parse method of the collection. This transforms the server response in the normal json format. Each item of the array is then passed to the parse method of the model.

The use of model.constructor is required, so that the right toJSON methods are used. Otherwise we do not support inheritance correctly... But this should not be a problem, if we work with json objects internally

[nilbus]

Hmm, I'll have to take ​a closer look. I'll look in more depth after I finish with what I'm working on with. Thanks for contributing this! It's definitely moving in the right direction.

[lsimoneau]

Just chiming in on this one, I got here because dualStorage wasn't playing nice with backbone.paginator, since Paginator overrides parse to deal with collection endpoints that include pagination data as a separate object, e.g.:

[
  { "total_pages":7, "page":1 },
  [
    { "id": 1, "title": "Lorem ipsum" },
    { "id": 2, "title": "Dolor sit amet"}
  ]
]

Requiring that users implement their own parse functions can be problematic in cases like this where parse is already overridden by another plugin. As far as I can tell this patch resolves the issue for me.