Skip to content

Conversation

@mattvague
Copy link
Contributor

@mattvague mattvague commented Sep 12, 2022

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 mattvague changed the title Feature/better json serialization deserialization Improved json serialization deserialization Sep 12, 2022
@mattvague mattvague changed the title Improved json serialization deserialization Improved json serialization / deserialization Sep 12, 2022
@josdejong
Copy link
Owner

josdejong commented Sep 16, 2022

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.

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

Successfully merging this pull request may close these issues.

2 participants