parse / toJSON applied inconsistent

[MichaReiser]

The parse and toJSON methods are not applied consistent.

• parseBeforeLocalSave should be defined on the model and is always invoked before the server response is saved in the local storage. This is not the case when a collection is fetched (the method is not invoked on the models).
• parse: Is always invoked when a record has been read from the local storage. It is not applied when the record has not bean read from localStorage. When a new model has been created, then the new model (initialized from it's default) value is passed to the parse method. But this is neither a server response nor a result from the local storage

I would expect:

• parseBeforeLocalSave: Is always invoked when we get a result from the server and before we write it to the localStorage. Maybe we should define this method as "static" so it's not based on an instance and you can invoke it using collection.model.parseBeforeLocalSave. The result of this method should be passed to the new collection.model(parsedAttributes)
• parse: Should always be invoked if we read something fron localStorage. In all other cases it should not be invoked (e.g. create new model from defaults)
• toJSON: Should only be invoked when we write a model to the localStorage or the server. It might be appropriate that we have here an additional overload toJSONBeforeRemote to allow the adoption of the server schema...

[nilbus]

Yes, this is a known issue. #53 was open to address this, but I'll keep this open instead as you have described the problem rather nicely.

I am tied up working on another feature now, but I would happily review and merge a pull request from you that addresses this problem.

[MichaReiser]

I'll try to have a look tomorrow.
But a review will be required anyway, because I don't know Backbone.dualStorage well enough

And someone needs to rewrite it in coffee script...

[nilbus]

Feel free to ask about anything you find confusing.

On Sat, May 31, 2014 at 3:58 PM, DatenMetzgerX notifications@github.com wrote:

I'll try to have a look tomorrow.

But a review will be required anyway, because I don't know Backbone.dualStorage well enough

— Reply to this email directly or view it on GitHub https://github.com/nilbus/Backbone.dualStorage/issues/111#issuecomment-44758134 .

[MichaReiser]

Coffee Script is confusing ;)

One question, when should parse be invoked...

From my point of view we should only use parse and toJSON. The dualStorage backend should "simulate" a server backend. In this case the state is saved to localStorag and it should behave the exact same way as a real server backend. This simplifies the programming (no new concept) and it is ensures, that a server response is parsed the same way as a local response. Additionally it doesn't requires any modification of the code if dualStorage is activated or not.

This means, when we store to localStorage we use toJSON for localStorage and the server backend.

When we read from localStorage or from the server we use parse to parse the "persisted" state from the remote site or the offline cached state. In both situation the result is the same. This makes Backbone.dualStorage transparent and doesn't need any additional considerations from the developer....

Have I missed a special case that needs to be handled? Or for what is the additional parseBefore... method? Why would I have two different schema (and parsing logic).

[nilbus]

Issue #2 discusses the rationale for parseBeforeLocalSave. It is useful in cases where you speak with a remote API that does not return an array of object attributes like Backbone expects. Normally parse would take care of this for you, but dualStorage caching happens before parse is/can be called. I implemented this feature as my first contribution to this project, and I believe I missed the things you pointed out. I think I agree with your expectations about how it should act, although I have not considered each point deeply.

[MichaReiser]

Ok, i see the use of the method now....

I think the behavior should be:

• parseBeforeLocalSave: Parses the request data and transforms the response into an intermediate result. Mainly used to adopt the server format. This format is used to store the entity in localStorage
• parse: Uses the intermediate result to parse. Performs the deserialization steps. The data can either be from parseBeforeLocalSave or the content from the localStorage
• toJSON: Serializes the entity to JSON. This data is used to persist the entity to the localStorage
• toJSONBeforeRemote: Only used when communicating with the server. It uses the result from toJSON and transforms the result to a server conform format.

The point is, we should never call parse in the dualStorage code, because backbone calls parse itself. If we call it too, its invoked multiple times. This means, we should only work with plain javascript objects when we work with the response and not with models. But we need models to be able to call the localSync "update" method...

So I think it's not so easy to fix this... The issue is in the core and not only on the surface.

[nilbus]

I think you're right.

For your understanding, I'll explain our previous rationale for working with the model instead of the JSON/attributes object. In order to know what attribute is the id to use as a storage key, we must know the model prototype's idAttribute. Since the idAttribute must accompany the response data, we decided to internally use and pass around the model object because it already contains both. It should be possible to instead pass around instead an object containing the model's attributes object and its idAttribute.

Right now we call parse in modelUpdatedWithResponse, which is used to update our locally stored attributes with responses from the server. The result from parse is immediately passed to set on a different, temporary model instance which is discarded and used only for its attributes and idAttribute when sent to localSync. Calling parse before set like we do here is consistent with the Backbone.Model.fetch implementation and is necessary to merge the response into the model's attributes using set like fetch does.

As you point out, we then take these parsed attributes and save them locally, which then later get parsed a 2nd time when they get fetched from localstorage.

Also as you point out, parseRemoteResponse is only called when reading from the remote in dualSync mode. The parseBeforeLocalSave filter is not applied as it ought to be when reading responses when saving a model, nor is it applied in remote: true onlineSync mode. This was the only problem that we knew about previously.

I think that moving away from using the model instance and toward using the attributes object paired with the idAttribute would allow us to solve the double parse problem.