-
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
fix(compiler): cannot use fromJson on jsii imported struct #4040
Conversation
@MarkMcCulloh @Chriscbr @yoav-steinberg Still have a bug to squash with 2 namespaces that have the same struct def (i.e. A.SomeStuct and B.SomeStruct) will cause collisions. But wanted to get some thoughts on these changes. The description has some notes about the big ideas with these changes. If you get a chance to review, please rip it apart 😁 I could not see a way around building a visitor and emitting structs defs before jsification phase. Its a pretty lightweight visitor. Im planning to address the namespace issue tomorrow. |
Console preview environment is available at https://wing-console-pr-4040.fly.dev 🚀 Updated (UTC): 2023-09-08 14:57 |
tools/hangar/__snapshots__/test_corpus/valid/struct_from_json.w_compile_tf-aws.md
Outdated
Show resolved
Hide resolved
17c5788
to
43f0b28
Compare
ironed out the namespaces bugs, and did some cleanups should be ready for review now |
@yoav-steinberg @Chriscbr Okay so I got in a groove today and might have gone crazy but I just did a refactor on this code. Please let me know your thoughts. Summary: I.E. The following wing code: struct Foo {
val: num;
val2: str;
}
Foo.fromJson({val: 10, val2: "10"}); Just emits this into the prefilight.js const Foo = $stdlib.std.Struct._createStructSchema({id:"/Foo",type:"object",properties:{val:{type:"number"},val2:{type:"string"},},required:["val","val2",]}); What I like about this is I no longer have to worry about file names and require calls. Also gets rid of a lot of boiler plate. Also I added the implementation for being able to call struct Foo {
val: num;
val2: str;
}
let schema = Foo.schema();
schema.validate({val: 10, val2: "10"});
log(schema.asStr()); |
@hasanaburayyan Looks much better! The inlining solution sounds good to me as well |
0139fdb
to
451f1bb
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.
I like this. Added a minor comment.
Didn't do an in depth review thought, this time. Hopefully good enough.
451f1bb
to
6413ca4
Compare
Thanks for contributing, @hasanaburayyan! This PR will now be added to the merge queue, or immediately merged if |
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
6413ca4
to
fd631c8
Compare
Thanks for contributing, @hasanaburayyan! This PR will now be added to the merge queue, or immediately merged if |
Congrats! 🚀 This was released in Wing 0.29.9. |
Summary
So this is actually quite a substantial refactor to how we generate jsonschemas for structs. Previously anytime we encountered a user defined wing struct we generated a json schema file with a schema and fromJson methods. However this meant that we were not created schema files for structs defined outside of the wing code (I.E. JSII imported). With the new changes I am taking a lazy creation approach where we only generate schemas for structs if there are references that would require the schema to exist.
This change also no longer generates a separate file for the schema, instead it creates an instance of a stdlib.std.StructSchema using the jsonschema inline.
The other big change is the removal of the require statements and defs from schemas that were dependent on other schemas. As in take for example this Wing struct:
The struct schema for
C
used to look like this:Now it looks like this:
Implementation Notes
Closes: #3943
Closes: #3790
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.