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

Questions about serialization / deserialization #15

Closed
martintietz opened this issue Feb 10, 2015 · 3 comments
Closed

Questions about serialization / deserialization #15

martintietz opened this issue Feb 10, 2015 · 3 comments

Comments

@martintietz
Copy link
Contributor

First I have to say that this is a great library. I'm currently using it in one of my projects and stumbled upon some smaller issues.

  1. Why do you "force" the user to return a String from a Store's serialize method? As far as I can see the whole state object is stringified anyway (https://github.com/acdlite/flummox/blob/master/src/Flux.js#L156). Let's say we have two Stores A and B, the result of flux.serialize() would look somewhat like this: JSON.stringify({ A: JSON.stringify(A.state), B: JSON.stringify(B.state) }) although those two inner stringifications (is that a word?!) are not necessary.
  2. In case of deserialization the flux class currently tries to parse the serialized state and if this works without an error it calls the deserialize function of each class. But you don't pass the parsed JSON as a function parameter to those calls, but the serialized state (https://github.com/acdlite/flummox/blob/master/src/Flux.js#L180) which means that each Store has to call JSON.parse again. This could be easier ;) You could even do something like that let storeState = deserialize(state[key]); and pass only the state to each store that is relevant for it.
@acdlite
Copy link
Owner

acdlite commented Feb 10, 2015

Hi! Thanks for the feedback.

I agree that this seems redundant if your stores only contain JSON stringifiable (is that a word?) data, but it's often the case that Store data is not JSON stringifiable — for instance, using Immutable.js data types. In that case, JSON.stringify() and JSON.parse() will not work, hence the current solution, which is just to let the user define how to serialize and deserialize state.

The calls to JSON.stringify() and JSON.parse() you've identified in the code are only for the top-level tree of store keys to store data strings. So the parsed object looks like this:

{
  storeA: '{"foo": "bar"}', // String of JSON stringified data, but it could actually be anything returned by `Store#serialize()`
  storeB: '{"foo": "bar"}'
}

Then we iterate over the values and send the strings to the stores for deserialization. In other words, Flux#deserialize makes no assumptions about the type of data in your stores.

Does that make sense? I'm open to suggestions for how to make this better.

@acdlite
Copy link
Owner

acdlite commented Feb 10, 2015

Ah! I read your comment again, and looked at the code again, and now I see the real issue — I'm passing the entire state string to Store#deserialize, not the store string! Oops! I'll fix this right away.

My previous comment is how it should work, though, so if you have any suggestions for how it could be better I'd still like to hear them :)

@acdlite
Copy link
Owner

acdlite commented Feb 10, 2015

Fixed in 2.5.1

@acdlite acdlite closed this as completed Feb 10, 2015
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

No branches or pull requests

2 participants