-
-
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
fix: Prevent type parsing to interfere with package name in typescrip… #1942
fix: Prevent type parsing to interfere with package name in typescrip… #1942
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.
Nice find and thanks for the PR! We are encouraging everyone to include tests with PRs to alert us to regressions at a later date. Do you think you could add a test to this PR? We have tests defined in v2/internal/binding/generate_test.go
.
Thanks 🙏
Yes, of course, I quickly tried, but testing this part is a not super straight forward. I'll see what I can do |
0941773
to
7c506ae
Compare
In the end, that was pretty easy. |
7c506ae
to
8878577
Compare
…t generation Before that fix: The method... ```go func (h *Handler) RespondToInteraction(interaction interactor.Interaction) {} ``` ... would generate... ```ts export function RespondToInteraction(arg1:number):Promise<Error>; ``` ... because the `interaction` package starts with `int` and anything starting with `int` is interpreted as `number`.
8878577
to
dee3388
Compare
I rebased against master. |
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.
Thanks for this. Added a comment. Thoughts?
b := binding.NewBindings(testLogger, []interface{}{&HandlerTest{}}, []interface{}{}, false) | ||
|
||
// then | ||
err := b.GenerateGoBindings(generationDir) |
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'm wondering if this would have been easier if we could generate to a map[string]string
and use that for testing. This would essentially represent map[filepath]filecontents
. Thoughts?
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.
Yes, that would have been easier.
I think we could significantly improve the design (and its testability) by splitting the bindings generation into multiple component (with Single Responsibility Principle, in mind).
I already had a look to hit, but wanted to keep this fix refactoring-free.
Here is just my brain dump on the matter.
Basically, the idea would be:
- A component that parse the go code and build an AST. That already exist AFAIK.
- A component that translate that Go AST into a JS AST
- Another component that translate that Go AST into TS AST
- A component responsible to write the JS AST as JS code
- Another component responsible to write the TS AST as TS code
The binding code would essentially become, as pseudo code:
func main() {
writer := fileWriter{}
goAST := GetGoASTFromBoundStructs({}interface{&Handler{}})
go func() {
ast := toJavascriptAST(goAST)
obfuscatedAST := obsfuscateJavascriptAST(ast)
code := toJavascriptCode(jsAST)
writer.Write(code, "Handler.js")
}()
go func() {
ast := toTypescriptAST(goAST)
obfuscatedAST := obsfuscateTypescriptAST(ast)
code := toTypescriptCode(ast)
writer.Write(jsCode, "Handler.d.ts")
}()
}
That example doesn't take into account the model generation, UPX and all the nitty gritty details. It's just to convey the idea.
Such design definitely will add more loop interaction but I believe the testability, extensibility (adding specific processing, features, etc) and ability to replace each component will increase dramatically.
Also, this is just a draft thought as I haven't looked in the details. I think I saw that part of the existing code is used for Wails runtime to call the methods and all, so it really needs more thoughts and design.
I am of course open to critics :)
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.
Let's look at refactoring separately 👍
Thanks @leaanthony ! I opened an issue about the refactoring proposal #1953. |
Before that fix:
The method...
... would generate...
... because the
interaction
package starts withint
and anything starting withint
is interpreted asnumber
as defined here:wails/v2/internal/binding/generate.go
Lines 130 to 134 in eee6797