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

Serialize + Parse Map Objects #823

Closed
tsengia opened this issue Sep 28, 2023 · 6 comments
Closed

Serialize + Parse Map Objects #823

tsengia opened this issue Sep 28, 2023 · 6 comments
Assignees
Labels
hacktoberfest help wanted Extra attention is needed question Further information is requested

Comments

@tsengia
Copy link
Contributor

tsengia commented Sep 28, 2023

📝 Summary

Right now I am finding that if typia.json.stringify() is used on an interface that has a Map object results in incorrect serialization, and most likely incorrect parsing as well.

  • Typia Version: 4.1.2
  • Expected behavior: typia.json.stringify() produces a string representation of the Map object
  • Actual behavior: Map object is reduced down to an empty object {} with no items

⏯ Playground Link

https://typia.io/playground/?script=JYWwDg9gTgLgBDAnmYBDOAzKERwORIqp4DcAUGcAHYwCmUGqAxrXALKICSN9jLcAbzJwRcEAC52qMAB4AzjCjUA5gBoFSqsoB8ZAL4UANrXipJHbnQbNWAXkFjJVWgHcpsjStVxPW7QAoASjg9clQAOhBwuRN-ACI6BTjVONQAIyYAE1oMOMDyMmN4ACs5CCoAfV9lOHtCNHDS8ujFFWAMRBkLHmsWANR8siZysuNwwwhleIApAGUAeQA5H1atSTi4AGo4JsrqwaK4MFQoGMzzLh6+OwRkBt3w1DkY2AAFE5iuy6trgN2q1bKQbDKijWjjSbxd6nVguaAAa1omQAhHlyCCwRCpscYZlItFgAAvWj5IA

💻 Code occurring the bug

import typia from 'typia';

interface MyInterface {
    m: Map<string,string>
}

let a: MyInterface = { m: new Map<string, string>() };
a.m.set("test","abcdef");

let json_string = typia.json.stringify<MyInterface>(a);
console.log("JSON string: " + json_string);
let parsed: MyInterface = typia.json.assertParse<MyInterface>(json_string);
console.log("Parse worked!");
console.log(parsed.m.size);

Map objects can use many different types for keys and values (see the MDN docs). But I think the most valuable for Typia to support would be string keys and values, as well as other basic primitives (integers/numbers, boolean values).

@tsengia tsengia changed the title DRAFT: Serialize + Parse Empty Map Object Serialize + Parse Map Objects Oct 5, 2023
@samchon
Copy link
Owner

samchon commented Oct 5, 2023

When call the native JSON.stringify() function to a Map, only "{}" string value be returned.

Therefore, it is not invalid serialization, but following the standard feature.

const map: Map<string, number> = new Map();
map.set("first", 1);
map.set("second", 2);

const str: string = JSON.stringify(map);
console.log(str);
{}

@samchon samchon self-assigned this Oct 5, 2023
@samchon samchon added the question Further information is requested label Oct 5, 2023
@tsengia
Copy link
Contributor Author

tsengia commented Oct 5, 2023

I see, would you be opposed to me adding a custom case for serializing Maps?

Or should users of Typia define their own custom Map-like class to perform equivalent serialize and deserialize?

At the very least, emitting a warning might be helpful, something along the lines of

Warning: ES5 Map() objects are not JSON serializable might be helpful

@samchon
Copy link
Owner

samchon commented Oct 5, 2023

Then how do you think about occuring compilation errors like below?

image

https://typia.io/docs/protobuf/message/#restrictions

@samchon samchon added the help wanted Extra attention is needed label Oct 5, 2023
@tsengia
Copy link
Contributor Author

tsengia commented Oct 6, 2023

I started down the path of "how would I implement serialize + parse?"

I dug into src/programmers/json/JsonStringifyProgrammer.ts but then realized that _test_json_stringify() would need to be considerably changed to support a case where the JSON.stringify() returns a value different from Typia's stringify due to this line here.

A possible solution would be to create a replacer function and pass it to all invocations of JSON.stringify() to allow JSON.stringify() to serialize Map objects. This could be extended to Sets as well.

Example replacer function:

let replacer = (key,value) => { 
    if(value.__proto__.constructor == Map) { 
        let s = []; 
        value.forEach((v,k,m) => { s.push([k,v]); }); 
        return s; 
    } 
    return value; 
}

let m = new Map([["hello","world"]]);

console.log(JSON.stringify(m, replacer)); // Outputs '[["hello","world"]]'

However, adding this would make Typia's JSON serialization not match JSON.stringify(), is that acceptable?
Or must Typia always match perfectly with JSON.stringify()?

If you do feel strongly about a potential difference between Typia and JSON.stringify(), then could this feature be an option that can be enabled in some project configuration file? : )

I am willing to implement this but just want your approval before I begin working on it.

@samchon
Copy link
Owner

samchon commented Oct 10, 2023

Not possible to supporting the custom replacer function through typia.

Also, I'm hoping typia.json.stringify<T>() to be exactly same with JSON.stringify() function. I don't want to add any custom conversion logic that can be different with the pure JSON.stringify() function, because we have to consider the opposite case - JSON.parse() function (or typia.json.assertParse<T>() function) call.

This is just my opinion at now - I think it would better to throw a compilation error when such Map type comes. But even this case, I'm very careful about that JSON.stringify() function can be called to the Map type even if it only returns the empty object bracket "{}".

@tsengia
Copy link
Contributor Author

tsengia commented Oct 10, 2023

Error or warning messages would be good then. I can see the desire to adhere to JSON.stringify()'s functionality.

In the end, I really should just convert all of my Map objects to plain old Arrays.

samchon added a commit that referenced this issue Oct 12, 2023
Close #823 - ban Map types on JSON functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants