search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.1k stars 336 forks source link

Expose `ToJson` and `FromJson` methods #885

Closed JounQin closed 2 years ago

JounQin commented 2 years ago

via https://github.com/mvdan/sh/issues/760#issuecomment-1120420888

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

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

mvdan commented 2 years ago

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

JounQin commented 2 years ago

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

Right, I need them in sh-syntax.

JounQin commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

mvdan commented 2 years ago

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 commented 2 years ago

Yep, partly supported.

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

mvdan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/tinygo-org/tinygo/issues/2115#issuecomment-1183304339.

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 commented 2 years ago

Great to hear!

mvdan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.