-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[v3] Binding generator improvements #3347
Conversation
Additionally: * fixes generation of tuple return type * improves imports and namespacing in JS mode * general cleanup of generated code
Improves support for unknown types (encoded as any) and maps (using Typescript index signatures)
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Thanks for doing this @fbbdev - Huge! I'll make my way through it in the coming days. |
@coderabbitai review |
@fbbdev - Initial passthrough looks good 👍 |
Testing this out against my test project on Manjaro now. |
I found a test case where no bindings are generated. |
Thank you @abichinger for finding this out. I pulled in your test and committed a temporary workaround that makes it pass, but a full solution would require a switch to the new type checking system. I am currently experimenting with that, I'll make a post as soon as I have something working. |
@leaanthony Thanks for having a look at it so soon! In general, I think it might be better to have a dedicated PR for documentation. We could start working on it right now if you wish. However, I think the current implementation has still quite some problems and we might want to consider changing something if things go well with the type checker. What do you think? |
Nice! Switching to the |
I can start that today. Just wanted to keep momentum 😅 |
Nice catch! |
Linked new docs PR |
Hi guys, I picked up the |
I guess this PR also solves #3056. Great job guys. 👍 |
I've been testing this with my project this week (macOS) and it's been great. It has resolved a couple of issues I've had with slice return types and returning both non-error and error values, both working as expected now. It also is now consistently working across the Awesome! 😃 The only thing that has come up is the |
Description
This is a large PR that brings many fixes and improvements to the binding generator. While the changes should be strictly non-breaking to the average user of the framework (with a very small exception, listed below), the edits will almost surely generate massive conflicts for people who might be concurrently working on the binding generator.
For that reason, after discussing this with @leaanthony, I am opening the PR in draft mode to wait for comments from the community. I am open to any suggestion and (reasonable) request and can collaborate if needed with people who might find it difficult to reconcile their work with the changes submitted here.
I think its best to keep the following description as short as possible, everybody please feel free to ask any question and I will expand on the details. Note that some commits contain additional explanations in the commit message.
Summary
The changes stack up on each other so it would be somewhat difficult to split them into separate PRs, but I tried to ensure every commit has consistent and working code, so it should be possible to merge up to some commit and delay later ones (apart from test updates, which are bundled in two final commits and will need to be redone). This should roughly correspond to keeping only an initial segment of the features listed above.
Support for complex types is still quite limited (for example there is no support for slices of slices, pointers to slices, and many other possible combinations). I just tried to fix the implementation for the most common cases. One way to work around this, for the time being, is to create a hierarchy of structs with simpler field types.
There are also many problems in the code that resolves imports and selector expression, but I'm leaving those out of this patch.
Assessing the patch
The best way to assess the final user-facing effects of the patch might be to inspect the diffs on testdata for the parser package and on the binding example with a good diff viewer (the VSCode one is fine, the github one doesn't look so bad either). The relevant commits are the last two.
Breaking changes
The only directly breaking change I am aware of is the following one:
Of course, people who came to depend on buggy behaviour may experience the fixes as breakage.
Main changes
The largest single change is probably be the move to templates for methods. All previous code has been thrown out and its behaviour reimplemented with Go templates. The code in
binding.go
is now minimal and quite similar to the code that was previously inmodels.go
, modulo some refactorings.The benefit of this change is that now there is a single authoritative source that defines the shape and content of generated JS code, so maintenance and improvement should become easier. The only other code that still generates some javascript is the
JS()
method on theParameterType
struct (subsuming the many duplicated instances that were lying around previously), and theDefaultValue()
method on theField
struct.The other important change is that now model generation is now driven by the same algorithm used by the encoding/json package for determining field names, visibility and other options. Json tags are now taken fully into account.
The benefit of this change is that people who respect generated types should see everything work as expected when passing structs into and out of bound methods.
Main fixes
The most important problem fixed by this patch is that the binding system used to Unmarshal all input arguments to the default types (i.e. maps) instead of the types expected by the methods, thus leading to runtime error. This is fixed by the first commit.
A connected problem was that the generated models relied upon a very simple heuristic for determining field names and visibility which did not match the one used by the encoding/json package, leading to errors and/or broken expectations. This motivated the second change described in the previous section.
A secondary problem was that the binding generator used to pass around package paths during generation and sometimes the wrong ones were passed (or package aliases in place of paths). This has been fixed by directly passing around pointers to package descriptors.
Type of change
How Has This Been Tested?
All binding generator tests, as well as the binding example, have been updated and verified to pass locally. Additional tests have been created for new functionality. All generated files have been carefully inspected by hand.
Test Configuration
Checklist:
website/src/pages/changelog.mdx
with details of this PR