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

Expose ToJson and FromJson methods #885

Closed
JounQin opened this issue Jun 25, 2022 · 15 comments · Fixed by #903
Closed

Expose ToJson and FromJson methods #885

JounQin opened this issue Jun 25, 2022 · 15 comments · Fixed by #903

Comments

@JounQin
Copy link
Contributor

JounQin commented Jun 25, 2022

via #760 (comment)

sh-syntax (WASM) is about 2x faster than mvdan-sh

https://github.com/rx-ts/sh-syntax/blob/main/benchmark/benchmark.txt

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

You mean as exported Go APIs, since currently -tojson is not?

@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

You mean as exported Go APIs, since currently -tojson is not?

Right, I need them in sh-syntax.

@JounQin JounQin changed the title Expose toJson and FromJson methods Expose ToJson and FromJson methods Jul 13, 2022
@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

A notice for WASM, we'd better not use reflect to generate json, because it could be broken, I'm using easyjson at sh-syntax, see https://github.com/un-ts/sh-syntax/blob/main/processor/structs_easyjson.go

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

At some point I do want to remove the dependency on reflect, but right now I want to stick with encoding/json for the sake of not pulling in large or buggy third-party json implementations. You're of course free to use your own implementation.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

You're of course free to use your own implementation.

I'm not sure how to. 😅

sh-syntax depends on this go library, if it depends on reflect in ToJson, how can I override it? Provide another copy on my side? Would it be hard to maintain?

Sorry I'm very new bee for go lang. Could you help to give me some advices?

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

Yes, I indeed mean not using these APIs if you cannot use reflect. So you probably shouldn't wait for them right now :)

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

I should also add that we should look into why our use of reflect wouldn't work. I am fairly sure that TinyGo does support part of reflect, for example.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

Yep, partly supported.

And we should also consider the final bundle size for WASM, that why we use tinygo, right?

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

Right, size matters. That's a reason to not use libraries like easyjson, in fact, because they are often many times bigger than encoding/json :)

@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

encoding/json + reflect is not fully supported by tinygo, so I have to use easyjson instead. 🤣

I tried but failed with native encoding/json + reflect with tinygo.

easyjson replaces reflect in this case.

@mvdan
Copy link
Owner

mvdan commented Jul 13, 2022

Looks like it is only blocked on reflect.StructOf (https://tinygo.org/docs/reference/lang-support/stdlib/#encodingjson), which I'm also using for the new version of "to JSON", since Go maps aren't ordered. I've left a comment at tinygo-org/tinygo#2115 (comment).

easyjson replaces reflect in this case.

Right, though easyjson uses code generation, so I'm fairly sure that the output size suffers because of it.

In the future, I can remove the use of reflection by swapping encoding/json with the token encoder and decoder in https://github.com/go-json-experiment/json. I don't want to do that right away, as I want to get "from JSON" working well first.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 13, 2022

Great to hear!

@mvdan
Copy link
Owner

mvdan commented Jul 17, 2022

Thinking outloud: this API should not be in the syntax package, because we don't want to require a JSON library to be able to parse shell.

@JounQin
Copy link
Contributor Author

JounQin commented Jul 17, 2022

Thinking outloud: this API should not be in the syntax package, because we don't want to require a JSON library to be able to parse shell.

What do you think should we do in sh-syntax package then?

@mvdan
Copy link
Owner

mvdan commented Jul 17, 2022

Nothing has to change there. It's just that instead of having the API like ToJSON(io.Writer, syntax.Node) in the mvdan.cc/sh/v3/syntax package, I'll probably put it somewhere else like mvdan.cc/sh/v3/syntax/typedjson. You can use both packages in your wasm code.

mvdan added a commit that referenced this issue Jul 19, 2022
Create a new package with docs, copy the code, and expose the API.
The following changes were made to the API:

* Name the main APIs Encode and Decode.
* Added options structs, to give us flexibility in the future.
* The "pretty" option is now an Indent string, like encoding/json.

Note that we now require "Type" keys to come first in each object.
This is not required right now, nor is it enforced by our decoder yet,
but it will be necessary to implement a performant decoder.
This is because we can only decode fields once we know the type;
if the "Type" key comes later, we must decode twice or buffer tokens.

The typed JSON encoding also changes slightly: since we want Encode and
Decode to work on syntax.Node rather than syntax.File, the root node
needs to include a "Type" JSON key as well.

This also makes shfmt's --to-json behave nicer on empty files;
rather than emitting `{}` lacking any information at all,
it now emits an empty file node in tthe form of `{"Type":"File"}`.

Finally, we apply some minor refactors to the code and tests.

Fixes #885.
mvdan added a commit that referenced this issue Jul 20, 2022
Create a new package with docs, copy the code, and expose the API.
The following changes were made to the API:

* Name the main APIs Encode and Decode.
* Added options structs, to give us flexibility in the future.
* The "pretty" option is now an Indent string, like encoding/json.

Note that we now require "Type" keys to come first in each object.
This is not required right now, nor is it enforced by our decoder yet,
but it will be necessary to implement a performant decoder.
This is because we can only decode fields once we know the type;
if the "Type" key comes later, we must decode twice or buffer tokens.

The typed JSON encoding also changes slightly: since we want Encode and
Decode to work on syntax.Node rather than syntax.File, the root node
needs to include a "Type" JSON key as well.

This also makes shfmt's --to-json behave nicer on empty files;
rather than emitting `{}` lacking any information at all,
it now emits an empty file node in tthe form of `{"Type":"File"}`.

Finally, we apply some minor refactors to the code and tests.

Fixes #885.
mvdan added a commit that referenced this issue Jul 20, 2022
Create a new package with docs, copy the code, and expose the API.
The following changes were made to the API:

* Name the main APIs Encode and Decode.
* Added options structs, to give us flexibility in the future.
* The "pretty" option is now an Indent string, like encoding/json.

Note that we now require "Type" keys to come first in each object.
This is not required right now, nor is it enforced by our decoder yet,
but it will be necessary to implement a performant decoder.
This is because we can only decode fields once we know the type;
if the "Type" key comes later, we must decode twice or buffer tokens.

The typed JSON encoding also changes slightly: since we want Encode and
Decode to work on syntax.Node rather than syntax.File, the root node
needs to include a "Type" JSON key as well.

This also makes shfmt's --to-json behave nicer on empty files;
rather than emitting `{}` lacking any information at all,
it now emits an empty file node in tthe form of `{"Type":"File"}`.

Finally, we apply some minor refactors to the code and tests.

Fixes #885.
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 a pull request may close this issue.

2 participants