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

Refactor of export to JSON function #175

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lorthiz
Copy link

@Lorthiz Lorthiz commented May 22, 2024

Problems I wanted to address that are present with current implementation:

  1. Mapping method was long and hard to understand, a lot of nested scopes(if driven development);
  2. Method had too much responsibility, it was traversing the tree, checking the types and finally writing a JSON string;
  3. JSON generated with hex flag was not possible to be parsed afterwards. JSON does not support HEX values, that is a domain of JSON5 that is not widely adapted;
  4. It was not easy to extend parsing to other formats since tree parsing was connected with JSON generation;

How I fixed them:
I split logic to 2 parts, one is a new Mapper that traverse the tree and simplifies it and only after that simplified object is mapped to a JSON string. This allows us to control how we want the final object to look like before it is mapped to JSON, but now we can also add exports to new formats without a need to copy/paste tree traversing.

One thing I wanted to ask: Was it somehow required for Uint8Byte arrays to be truncated to 64 elements? I left that part "as is", but we would change it would make it possible to have the whole structure of the object as it is represented in the tree: so users could export raw JSON directly to their projects without a need to generate the parser and load a file again afterwards. It would also fix this issue:

#147

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.

1 participant