-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(compiler): support json to struct conversion #3648
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work
added some questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 👏
I was wondering if it might make more sense to generate a json schema and use a standard schema validation utility. We also want to support something like: let s = MyStruct.jsonSchema();
s.validate()
s.toJson()
// etc.. The use case is for example using wing structs as api shapes with OpenAPI specs. |
After reviewing I thought of this too. I think we should have a var v = new require('jsonschema').Validator(); // see https://www.npmjs.com/package/jsonschema
v.validate(obj, self.getSchema()); If there's no major issue with this approach I suggest doing another dev push to making it work this way and void our custom validation code. |
@yoav-steinberg @eladb When I originally started I wanted to generate schemas and went down a few learning rabbit holes, and did not think it was something we could easily do (because of nested arrays that can contain user defined structs), thats when I shifted gears to this approach. However, back then I was younger, naive and since then I have grown and learned more about the way bundling and some of the other JavaScript vodoo works 😂 😂 and I think generating these schemas for use in a standard validator can be "easishly" done. Im going to convert this PR to a WIP and investigate changing to that pattern (assuming there are not any objections) |
1c1f230
to
4e057c3
Compare
tools/hangar/__snapshots__/test_corpus/valid/struct_json_conversion.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
4e057c3
to
a95c606
Compare
tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/events.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work using schemas
tools/hangar/__snapshots__/test_corpus/valid/struct_json_conversion.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
tools/hangar/__snapshots__/test_corpus/valid/structs.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
tools/hangar/__snapshots__/test_corpus/valid/struct_json_conversion.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
0af2e0e
to
31c0990
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6915625
to
0b764ca
Compare
Added do not merge label until I complete the documentation and issue tracking todos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey!
This is amazing and I know you are keen to get this over the finish line, but there are a few tweaks I think we should make.
The generated struct code and the interaction between it and the std.Struct seem a bit convoluted.
I imagined something like;
Foo.Struct.js
class Foo {
static get schema() {
// return a full self-contained json schema, including all nested structs...
}
static fromJson(x) {
return std.Struct.validate(x, this.schema);
}
// lifting boilerplate
}
Something along these lines. KISS...
tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/events.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/events.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
tools/hangar/__snapshots__/test_corpus/valid/struct_from_json.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
That's usually something I regret, especially with a big project like this... I would try to get this merged first and follow up with subsequent PRs. |
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
1e55669
to
cd67322
Compare
d3ab970
to
36b651d
Compare
Signed-off-by: monada-bot[bot] <monabot@monada.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @yoav-steinberg / @Chriscbr / @MarkMcCulloh please give the final approval
tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/events.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
Signed-off-by: monada-bot[bot] <monabot@monada.co>
Signed-off-by: monada-bot[bot] <monabot@monada.co>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Congrats! 🚀 This was released in Wing 0.25.1. |
The changes in this PR make it possible to call
fromJson
on any compatible struct definition.Implementation notes:
Previously we treated the jsification of structs as a no-op since there is not a JS equivalent. So with this change structs now JSify into a
Struct
file that contains a class with a staticjsonSchema
and afromJson
function which will allow for field validation at runtime.The schema generated adheres to: https://json-schema.org/understanding-json-schema/
take this simple example Wing code:
this will now generate a JS file named
MyStruct.Struct.js
which looks like this:The piece that brings this all together is the addition of the
Struct
class in our std that only has afromJson()
methods at the moment that is a Wing macro. The macro just calls thefromJson()
method in the generated javascript.Misc
We want to stop the user at compile time from calling
fromJson
on a struct that cannot be represented by a Json value ieto prevent this I added a check in the typechecker for structs to confirm that if
fromJson
is called that all the fields in the struct are valid for conversion attemptSee image below for error when attempting:
Closes: #3653
Closes: #3139
TODO:
Followup issues that are out of scope for this PR:
Checklist
pr/e2e-full
label if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.