Make exporting all exported fields an explicit config. #3970
Conversation
WalkthroughThe pull request introduces a new configuration option Changes
Assessment against linked issues
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying wails with
|
| Latest commit: |
5a078db
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fe391d15.wails.pages.dev |
| Branch Preview URL: | https://v2-bugfix-explicit-json-tags.wails.pages.dev |
|
@pbnjay - please review this as it will revert your previous changes by default. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
v2/pkg/commands/bindings/bindings.go (1)
18-28: Add doc commentary for clarity on the new field
It's good to seeGenerateAllExportedFieldsincluded in theOptionsstruct. Consider adding a brief comment above this field to clarify its usage for other developers and ensure consistent code documentation practices.v2/internal/app/app_bindings.go (2)
35-35: Variable name consistency
The namegenerateallexportedfieldsFlagis clear but quite verbose. It might be shortened (e.g.,exportedFieldsFlag) to improve readability while retaining meaning.
79-79: Add checks or logging for the new configuration
The call toSetGenerateAllExportedFieldsis straightforward. Consider adding minimal logging or debug info, so users know if they’ve enabled this feature.v2/internal/project/project.go (1)
241-242: Add short comment explaining the field’s purpose
The newGenerateAllExportedFieldsconfig key is valuable. Add a top-level doc comment for future maintainers to understand its intended usage and side effects.v2/internal/binding/binding.go (2)
25-31: Add doc comment for clarity.
You’ve added a new fieldgenerateAllExportedFieldsto theBindingsstruct in the midst of other fields. Consider adding a brief comment above it (and possibly the other fields) to explain its purpose and effect on generated bindings.
331-334: Fluent setter chaining.
The fluent approach inSetGenerateAllExportedFieldsis good. For clarity, consider reflecting that in the method’s doc comment (e.g., “Returns *Bindings so calls can be chained.”).v2/internal/typescriptify/typescriptify.go (1)
108-109: Document newly added field.
TheGenerateAllExportedFieldsfield is a key addition. A brief comment about its influence on struct field processing would aid maintainability.website/static/schemas/config.v2.json (1)
259-263: LGTM! Consider adding examples to improve documentation.The new configuration option is well-structured and correctly implements the opt-in behavior for generating all exported fields. The default value of
falseappropriately addresses the breaking change concerns from issue #3809.Consider enhancing the documentation by adding examples showing both scenarios (with and without the flag enabled):
"generate_all_exported_fields": { "type": "boolean", "default": false, - "description": "When set to true, all exported fields will be generated, even if they have no JSON tag." + "description": "When set to true, all exported fields will be generated, even if they have no JSON tag.", + "examples": [ + { + "bindings": { + "generate_all_exported_fields": true // Generate TypeScript models for all exported fields + } + }, + { + "bindings": { + "generate_all_exported_fields": false // Generate TypeScript models only for fields with JSON tags + } + } + ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
v2/cmd/wails/generate.go(1 hunks)v2/internal/app/app_bindings.go(4 hunks)v2/internal/binding/binding.go(5 hunks)v2/internal/project/project.go(1 hunks)v2/internal/typescriptify/typescriptify.go(2 hunks)v2/pkg/commands/bindings/bindings.go(2 hunks)v2/pkg/commands/build/build.go(1 hunks)website/docs/reference/project-config.mdx(1 hunks)website/static/schemas/config.v2.json(1 hunks)
🔇 Additional comments (10)
v2/pkg/commands/bindings/bindings.go (1)
86-86: Ensure consistency of environment variable name
This line sets the generateallexportedfields environment variable. Confirm it remains synchronized with references in other files (e.g., app_bindings.go) to avoid confusion.
v2/internal/app/app_bindings.go (2)
14-16: Check error handling around environment loading
You’ve added strconv for parsing, which is good. However, consider handling potential parsing errors in a more user-friendly manner or providing a default fallback to avoid abrupt exits if env vars are invalid.
52-62: Defaulting logic
Well done on providing a default false when the environment variable is unset. This aligns with the PR objective to disable the feature unless explicitly enabled.
v2/cmd/wails/generate.go (1)
51-56: Leverage the new option in run tests
Including GenerateAllExportedFields in bindings.Options is a good step. Ensure you update or add integration tests that cover both the enabled and disabled states of this option, preventing regressions.
v2/internal/binding/binding.go (3)
110-110: Ensure consistent default.
The assignment to w.GenerateAllExportedFields is straightforward. Just confirm that the default value is correct if it’s never modified (i.e., false).
162-162: Maintain parity across usage sites.
Reaffirm that all references to w.GenerateAllExportedFields (here and in line 110) share consistent logic so that exported fields in enumerated packages are handled uniformly.
361-367: Validate fields without JSON tags.
Including exported fields that lack a JSON tag can be helpful. Just ensure that it doesn’t inadvertently create collisions with fields that already have JSON tags. Consider logging or raising conflicts if encountered.
v2/pkg/commands/build/build.go (1)
231-239: Populate new option carefully.
Passing GenerateAllExportedFields here looks good. Confirm that its default in project config (false) is retained if not explicitly set, and that all usage sites expect the boolean properly.
v2/internal/typescriptify/typescriptify.go (1)
559-566: Review field naming collisions.
Automatically assigning jsonFieldName to the exported field name if no JSON tag exists can be handy. Maintain caution around potential naming collisions (e.g., “Field” vs. “field”).
website/docs/reference/project-config.mdx (1)
112-113: Clarity in documentation.
The documentation for generate_all_exported_fields is clear, but consider explicitly calling out the default value (false) and the potential side effects on the TypeScript models.
|
I feel like #3809 is two edge cases missed in the original PR (I’ll help debug). but reverting the default behavior here isn’t ideal for new users since it will not match existing Go json semantics. |
|
Whilst I agree that it should have been closer aligned to Go's JSON semantics to begin with, it doesn't change the fact that it's still a breaking change. This behaviour is default in v3 so this is only an issue for v2. |
|
It’s your project so up to you, but I guess I’m confused that the original PR is considered a “breaking” change. The linked issue is a bug for sure, but my understanding is the original PR was backwards compatible since at worst it exposes fields already considered exported? |
|
I'm ok with not merging this if we can find a suitable solution for the bug report. |
Description
A breaking change was introduced in #3678 which has caused issues for some people: #3809 . This PR disables this feature by default, but adds a project option to enable it.:
New
wails.jsonoption:Fixes #3809
Summary by CodeRabbit
Release Notes
New Features
generate_all_exported_fieldsallows more flexible field generationDocumentation
The new feature provides developers with enhanced control over TypeScript binding generation, allowing inclusion of all exported fields even without explicit JSON tags.