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

Value::to_json() does not handle undefined correctly #403

Closed
4 tasks
n14little opened this issue May 15, 2020 · 3 comments · Fixed by #402
Closed
4 tasks

Value::to_json() does not handle undefined correctly #403

n14little opened this issue May 15, 2020 · 3 comments · Fixed by #402
Labels
bug Something isn't working
Milestone

Comments

@n14little
Copy link
Contributor

n14little commented May 15, 2020

I'm currently working on the JSON.stringify replacer and I noticed something that caught me a little bit off guard. When I call someValue.to_json() it is not removing properties whose values are undefined. Since value.to_json() and JSON.stringify(...) are both taking things and turning them into "JSON", I would expect them to behave similarly. MDN gives this blurb on the JSON.stringify(...) page:

undefined, Functions, and Symbols are not valid JSON values. If any such values are encountered during conversion they are either omitted (when found in an object) or changed to null (when found in an array). JSON.stringify() can return undefined when passing in "pure" values like JSON.stringify(function(){}) or JSON.stringify(undefined).

However, Boa's to_json function blindly turns null, symbols and undefined all into json null values.

https://github.com/jasonwilliams/boa/blob/402041d43251017ab772fe6f9920e41f496a359d/boa/src/builtins/value/mod.rs#L685

If this is very intentional behavior I can certainly do some checks in the JSON.stringify() to not add properties whose values are undefined, but thought I would pose the question since putting it in the to_json function would reproduce that behavior throughout the codebase.

  • does not remove key/value pairs whose value is undefined
  • does not remove key/value pairs whose value is a function
  • does not remove key/value pairs whose value is a symbol
  • json stringify arrays does not work properly
    e.g.
const y = [1, 2, 3];
console.log(JSON.stringify(y));
// Boa logs
// {"0":1,"1":2,"2":3,"length":3} --- INCORRECT

// Should log
// "[1, 2, 3]"
@jasonwilliams
Copy link
Member

Hey thanks for raising this, I don’t think it is intentional behaviour it most likely hasn’t been dealt with. So if we should be omitting instead of Nulling them then go ahead on making that change.

@Razican Razican changed the title Value to_json Value::to_json() does not omit null or undefined values May 16, 2020
@Razican Razican added the bug Something isn't working label May 16, 2020
@n14little
Copy link
Contributor Author

@Razican to clarify, I do not believe that Value::to_json should omit null, nor should it always omit undefined. It depends on the situation. For example, when undefined is found as a value in an object, that key/value pair should be omitted; however, when undefined is found in an array, then it should be converted to null. Does that make sense?

@Razican Razican changed the title Value::to_json() does not omit null or undefined values Value::to_json() does not handle undefined correctly May 16, 2020
@Razican
Copy link
Member

Razican commented May 16, 2020

@Razican to clarify, I do not believe that Value::to_json should omit null, nor should it always omit undefined. It depends on the situation. For example, when undefined is found as a value in an object, that key/value pair should be omitted; however, when undefined is found in an array, then it should be converted to null. Does that make sense?

I updated the title ;)

@Razican Razican added this to the v0.9.0 milestone Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants