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

Support JSON5 deserialization #160

Closed
wants to merge 5 commits into from

Conversation

CompeyDev
Copy link
Contributor

@CompeyDev CompeyDev commented Feb 25, 2024

This PR implements JSON5 deserialization (and serialization too, for the sake of completeness) for the serde and net builtin libraries.

TODO:

  • Implementation
    • @lune/serde
      • encode
      • decode
    • @lune/net
      • jsonDecode
  • Tests & Documentation
    • Document new json5 union variant
    • Include tests

Closes #141.

JSON5 is a superset of JSON, which includes support for trailing commas,
extra whitespaces, comments, and an expanded set of numbers. This commit
adds mainly decoding support for json5, in `@lune/serde`. Encoding
"support" is also included, but isn't a practical use-case.

* Expanded `EncodeDecodeFormat` to have a `Json5` variant
* Handled JSON5 decoding using `serde_json5` crate
* Matched JSON5 encoding alongside JSON
@filiptibell
Copy link
Collaborator

It seems this PR conflates two different standards (json and json5) in a couple places. In previous discussion on Discord I also rejected the proposal to add json5, with the two main points being:

  • The serde_json5 crate is for all intents and purposes unmaintained, and is a fork of an even older, even less maintained crate with many pending issues
  • Any changes they have made/accepted since the initial fork release, seem to not have been published

A better option here would be "json with comments", using something like https://crates.io/crates/jsonc-parser. This is human-friendly JSON that doesn't try to include lots of extra ECMAScript syntax.

@CompeyDev
Copy link
Contributor Author

It seems this PR conflates two different standards (json and json5) in a couple places.

Could you elaborate? JSON is technically valid JSON5, and so encoding is the only place I've used them interchangeably.

@CompeyDev
Copy link
Contributor Author

Also, if you could reopen the PR, I guess I could replace the existing changes to make use of JSONC instead, considering the changes were seemingly trivial.

@filiptibell
Copy link
Collaborator

Could you elaborate? JSON is technically valid JSON5, and so encoding is the only place I've used them interchangeably.

For net.jsonDecode, using json5 brings a performance penalty, it's also not really used for sending over the network and I don't think we should encourage that. The json5 spec repo also says the same.
Serializing both json and json5 as plain json also breaks some expectations - keys should be bare strings whenever possible with json5 according to their spec tests, not quoted like json. I'm not sure if serde_json5 does this or not though but if it doesn't that's another minus point for it

Also, if you could reopen the PR, I guess I could replace the existing changes to make use of JSONC instead, considering the changes were seemingly trivial.

I would like to keep this one closed so that it is searchable/linkable in the future, but feel free to make another PR

@CompeyDev
Copy link
Contributor Author

Thanks for the clarification, guess I'll create a new PR in the future once I've finished the ones I'm currently working on.

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.

Add JSON comment support to Lune APIs
2 participants