mathjs icon indicating copy to clipboard operation
mathjs copied to clipboard

Improved json serialization / deserialization

Open mattvague opened this issue 3 years ago • 1 comments

This PR addresses this issue albeit in a slightly different way than was described. Instead of implementing toObject and fromObject I opted to instead:

  1. Implement the fromObject function (which I'm calling fromJSON since I think that makes its purpose more obvious)
  2. Altered the existing toJSON methods on each node type to deeply serialize to JSON.
  3. Altered the existing fromJSON methods on each node type to deeply deserialize from JSON

Note I'd be happy to add a generic toJSON function as well but I honestly am not sure I see the value when you can always just call node.toJSON(), but maybe I'm missing something

  1. Altered the existing toJSON methods on each node type to deeply serialize to JSON.
  2. Altered the existing fromJSON methods on each node type to deeply deserialize from JSON

Note @josdejong I wasn't sure if there was a reason these methods didn't work "deeply" before, but it seems to me like they would be most useful this way no?

mattvague avatar Sep 12 '22 20:09 mattvague

Thanks Matt for picking this up 👍

The reason that there is no recursive implementation of .toJSON() is that this is handled by JSON.stringify already. The docs explain how to use it:

Docs: https://mathjs.org/docs/core/serialization.html

So right now, JSON.stringify (and math.replacer) is your generic toJSON function :). I think we can do the same for the new functions fromObject and toObject, so let them handle the iteration and call the fromJSON and toJSON methods. What do you think?

I think if you use JSON.stringify and let the toJSON methods to do the iteration themselves too, it will iterate twice, which is bad for performance.

josdejong avatar Sep 16 '22 10:09 josdejong