Skip to content
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

Ability to define prefix / suffix for TS module #2101

Merged
merged 36 commits into from Nov 24, 2022

Conversation

OlegGulevskyy
Copy link
Contributor

As discussed in the issue #2058 , this PR adds ability for the app developer to define prefix and suffix for their return entities from the backend. Those pref/suff will be used in the generated models.ts file, to avoid clash with JS reserved keywords.

  • Added a full list of JS keywords and detection of the clash
  • Added warning message for each type to the console log when the types are being converted. A note here: - I am not exactly happy how this warning implemented (it's a simple log and outputting it into stderr but I don't know any other better way, I'd love some guidance here.
  • Made sure prefix / suffix are added in all modes - wails dev / wails build / wails generate module. Please let me know if I missed any other.

Some implementation details question:

  1. Prefix and Suffix flags are implemented as structs that can be reimported (they have Flag and Description properties). Is this ok thing to do in Go lang? I thought of treating it as enum, because retyping it in all other places is error prone and if the flag needs to be changed - it can be just changed in one place and affect all the other.
  2. What is the better way to implement warning logging for cases when there is a clash with JS reserved keyword?

Please don't hesitate to say what's not idiomatic way to do things, what does not make sense or what does, I am a webdev with JS/TS background and Go is my new tool in the toolbox, so I am just learning and open to any feedback!

Thanks in advance

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments 👍

v2/internal/typescriptify/typescriptify.go Show resolved Hide resolved
"github.com/wailsapp/wails/v2/internal/fs"
"github.com/wailsapp/wails/v2/internal/process"
"github.com/wailsapp/wails/v2/pkg/clilogger"
"github.com/wailsapp/wails/v2/pkg/commands/build"
"github.com/wailsapp/wails/v2/cmd/wails/internal/utils"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go, you generally don't have ambiguous package names. In this instance you may want to call it "logutils" or something. It actually doesn't bother me so much, more an FYI 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great tip, thank you! will refactor it and keep it in mind!

@stffabi
Copy link
Collaborator

stffabi commented Nov 17, 2022

Awesome work, thanks @OlegGulevskyy 👍

Just a note which might be worth to be discussed:
I wonder if those settings should be put into wails.json. I think for most people that need to solve the ambiguity with JS keywords want this to be setup once for their project. Otherwise they would need to remember to define those options for every wails dev and for every wails build call, because we now generate the bindings for every build. Also if multiple people are working on one project they would need to remember those settings. And one needs to make sure to update the build server definition as well, if a company is using build agents. If it was defined in wails.json this would only need to be defined once and every one using that project would use the correct settings.

Thoughts?

@leaanthony
Copy link
Member

That's a great call actually.

@OlegGulevskyy
Copy link
Contributor Author

Agreed, that's a good call!
Looking at schema here, what would you say the properties could be?
My initial thinking is something like this
image

@leaanthony
Copy link
Member

Perhaps ts_generation?

@OlegGulevskyy
Copy link
Contributor Author

ts_generation works for me. What do you think? @stffabi

@stffabi
Copy link
Collaborator

stffabi commented Nov 17, 2022

ts_generation works for me. What do you think? @stffabi

Yeah that sounds good for me👍

@OlegGulevskyy OlegGulevskyy requested review from leaanthony and removed request for leaanthony November 18, 2022 13:51
@OlegGulevskyy
Copy link
Contributor Author

Updated PR to use wails.json file to configure this one + updated json schema
Let me know if it looks good to you and I can update changelog

@leaanthony
Copy link
Member

LGTM. One thing I'm going to suggest is a test in the binding_test directory to ensure it works as intended and we don't get regressions in the future 👍

@OlegGulevskyy
Copy link
Contributor Author

Had busy few days with work, but got your comment, I haven't forgotten and will look into it! :)

@OlegGulevskyy
Copy link
Contributor Author

Added a simple use cases for automated testing, to make sure the entity is generated with prefix and suffix whenever one or the other is supplied.
Let me know if you'd like me to cover some other cases :)

Thanks for your feedback and guidance, I appreciate it!

cc @leaanthony @stffabi

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small suggestions 👍

website/static/schemas/config.v2.json Outdated Show resolved Hide resolved
website/static/schemas/config.v2.json Outdated Show resolved Hide resolved
v2/internal/typescriptify/typescriptify.go Show resolved Hide resolved
@leaanthony
Copy link
Member

leaanthony commented Nov 23, 2022

Also, please update the CHANGELOG in the website directory so it isn't missed next release 👍🙏

OlegGulevskyy and others added 5 commits November 23, 2022 21:06
Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
@OlegGulevskyy
Copy link
Contributor Author

Sure, all done! Let me know if the wording in changelog needs tweaking

@leaanthony leaanthony merged commit ca8a1fa into wailsapp:master Nov 24, 2022
@leaanthony
Copy link
Member

Awesome first issue PR! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants