refactor(gql): migrate to IR-based code generation architecture#73
Conversation
Replace individual language generator scripts with unified IR-based plugin system for better maintainability and consistency. Architecture: - GraphQL Schema → Parser → IR (Intermediate Representation) → Plugins - Single parsing pass instead of 5 separate parsers - Language-agnostic IR types: IREnum, IRObject, IRUnion, etc. New plugin system (codegen/plugins/): - base-plugin.ts: Abstract base class with shared logic - swift.ts: Codable protocol, ErrorCode handling - kotlin.ts: Sealed interface, fromJson/toJson - dart.ts: Sealed class, factory constructors - gdscript.ts: Godot engine types, from_json/to_json Deleted redundant scripts (3,687 lines removed): - generate-swift-types.mjs (831 lines) - generate-kotlin-types.mjs (939 lines) - generate-dart-types.mjs (1,105 lines) - generate-gdscript-types.mjs (812 lines) Generated output is 100% identical to previous scripts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new IR-based GraphQL code generation system in TypeScript: parser, AST→IR transformer, IR types, utilities, template engine, plugin framework, language plugins (Swift/Kotlin/Dart/GDScript), Handlebars templates, CLI entry, docs, and removes legacy per-language MJS generator scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodeGen as CodeGenerator
participant Parser as SchemaParser
participant Transformer as SchemaTransformer
participant Plugin as CodegenPlugin
participant FS as FileSystem
User->>CodeGen: generate(config)
CodeGen->>Parser: parseSchema(config)
Parser->>FS: read SDL files
FS-->>Parser: SDL contents
Parser-->>CodeGen: ParsedSchema (schema, markers, sdlContents)
CodeGen->>Transformer: transformSchema(ParsedSchema)
Transformer-->>CodeGen: IRSchema
loop For each language
CodeGen->>CodeGen: createPlugin(language)
CodeGen->>Plugin: plugin.generate(IRSchema)
Plugin->>Plugin: mapType()/mapScalar() / render templates
Plugin-->>CodeGen: generated code (string)
CodeGen->>FS: write output file
end
FS-->>User: generated files
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @hyochan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the GraphQL code generation system by introducing an Intermediate Representation (IR) layer. This change centralizes the schema parsing and transformation logic, enabling a more modular and efficient approach to generating client-side types for multiple programming languages. The new architecture simplifies the addition of new languages and improves the overall maintainability of the code generation process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive and well-executed refactoring of the code generation system. Migrating from separate scripts to a unified, IR-based plugin architecture is a significant improvement for maintainability and extensibility. The new structure is clean, well-documented, and the language-specific plugins demonstrate a strong understanding of idiomatic patterns for each target language (Swift, Kotlin, Dart, and GDScript). The extensive documentation updates across the repository are also a huge plus.
I have a couple of minor suggestions for further improvement, mainly around consistency in the Dart generator and cleaning up some potentially unused code. Overall, this is a fantastic contribution that sets a solid foundation for future schema evolution.
|
|
||
| private generateResultUnionObject(irObject: IRObject): void { | ||
| this.generateDocComment(irObject.description); | ||
| this.emit(`abstract class ${irObject.name} {`); |
There was a problem hiding this comment.
In generateResultUnionObject, you're using abstract class for the result union base class. For consistency with other parts of the Dart generator (like generateUnion and the special handling for RequestPurchaseProps) and to leverage better static analysis, consider using sealed class instead. This will enforce compile-time exhaustiveness checking for consumers of these types.
| this.emit(`abstract class ${irObject.name} {`); | |
| this.emit(`sealed class ${irObject.name} {`); |
There was a problem hiding this comment.
Thank you for the suggestion. However, this PR's goal is to maintain 100% identical output to the previous generator scripts, which used abstract class for result union types.
Changing to sealed class would be a breaking change for existing consumers and is outside the scope of this refactoring PR.
This would be a great enhancement for a follow-up PR that explicitly bumps the version and documents the breaking change.
| /** | ||
| * Template-based Plugin for Code Generation | ||
| * | ||
| * Base class that uses Handlebars templates for common structures | ||
| * while allowing language-specific customization through hooks. | ||
| */ | ||
|
|
||
| import Handlebars from 'handlebars'; | ||
| import { readFileSync } from 'fs'; | ||
| import { join, dirname } from 'path'; | ||
| import { fileURLToPath } from 'url'; | ||
| import { CodegenPlugin, type CodegenPluginConfig } from './base-plugin.js'; | ||
| import type { | ||
| IRSchema, | ||
| IREnum, | ||
| IRInterface, | ||
| IRObject, | ||
| IRInput, | ||
| IRUnion, | ||
| IROperation, | ||
| IRType, | ||
| IRField, | ||
| IROperationField, | ||
| } from '../core/types.js'; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = dirname(__filename); | ||
|
|
||
| // ============================================================================ | ||
| // Template Context Types | ||
| // ============================================================================ | ||
|
|
||
| export interface EnumValueContext { | ||
| name: string; | ||
| caseName: string; | ||
| rawValue: string; | ||
| camelCaseName: string; | ||
| description?: string; | ||
| legacyValues: string[]; | ||
| isLast: boolean; | ||
| } | ||
|
|
||
| export interface FieldContext { | ||
| name: string; | ||
| graphqlName: string; | ||
| propertyName: string; | ||
| paramName: string; | ||
| type: string; | ||
| declarationType: string; | ||
| description?: string; | ||
| nullable: boolean; | ||
| isOverride: boolean; | ||
| defaultValue: string; | ||
| annotation: string; | ||
| fromJsonExpr: string; | ||
| toJsonExpr: string; | ||
| isLast: boolean; | ||
| } | ||
|
|
||
| export interface OperationFieldContext { | ||
| name: string; | ||
| escapedName: string; | ||
| propertyName: string; | ||
| paramName: string; | ||
| description?: string; | ||
| returnType: string; | ||
| args: ArgContext[]; | ||
| hasArgs: boolean; | ||
| hasSingleArg: boolean; | ||
| hasMultipleArgs: boolean; | ||
| aliasName: string; | ||
| argsSignature: string; | ||
| paramsSignature: string; | ||
| isLast: boolean; | ||
| } | ||
|
|
||
| export interface ArgContext { | ||
| name: string; | ||
| escapedName: string; | ||
| type: string; | ||
| defaultValue: string; | ||
| isLast: boolean; | ||
| } | ||
|
|
||
| export interface ConcreteMemberContext { | ||
| typeName: string; | ||
| delegateTo: string; | ||
| isNested: boolean; | ||
| wrapperName: string; | ||
| } | ||
|
|
||
| export interface NestedUnionWrapperContext { | ||
| wrapperName: string; | ||
| unionName: string; | ||
| parentUnionName: string; | ||
| interfaceFields: FieldContext[]; | ||
| } | ||
|
|
||
| export interface ResultUnionEntryContext { | ||
| fieldName: string; | ||
| caseName: string; | ||
| className: string; | ||
| type: string; | ||
| isLast: boolean; | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Template Plugin Base Class | ||
| // ============================================================================ | ||
|
|
||
| export abstract class TemplatePlugin extends CodegenPlugin { |
There was a problem hiding this comment.
This TemplatePlugin and the associated Handlebars templates in packages/gql/codegen/templates/ appear to be unused. The language-specific plugins (swift.ts, kotlin.ts, etc.) all extend CodegenPlugin directly and build the code strings programmatically.
If this template-based approach is a remnant of a previous implementation or intended for future use, it might be worth adding a note about its status. If it's no longer needed, consider removing it and the templates directory to reduce codebase size and maintenance overhead.
There was a problem hiding this comment.
The template system was explored as a potential approach during this refactoring, but was found to be impractical for the full implementation due to complex language-specific requirements (JSON serialization, custom type handling, platform defaults).
However, the templates are intentionally kept for:
- Future extensibility - Simple structural generation may benefit from templates
- Documentation - They serve as a reference for the expected output format
- Potential hybrid approach - Some simpler parts could be migrated later
I've added a note in the README explaining this status. If you feel strongly that they should be removed, happy to do so in a follow-up PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@packages/gql/codegen/core/parser.ts`:
- Around line 142-146: The current check for future markers is too broad and
uses trimmed.toLowerCase().includes('future'); change it to match only a
stricter pattern so we don't false-positive on comments like “For future
reference.” In the parser.ts snippet that sets expectFutureField, replace the
includes check with a case-insensitive whole-word/start-of-comment match (e.g.,
match lines that begin with '#' followed by optional whitespace and the word
"future", optionally followed only by typical separators like ':' or '-' and an
inline note). Keep the logic tied to the same variables (trimmed and
expectFutureField) so only explicit "Future" markers set expectFutureField.
In `@packages/gql/codegen/core/template-engine.ts`:
- Around line 179-187: The 'and' and 'or' Handlebars helpers use arrow functions
which capture the outer this and thus pass the wrong context to
options.fn/options.inverse; change both to regular function expressions (e.g.,
function(...args: unknown[]){ ... }) so Handlebars' invocation context is bound
correctly, keep the logic that pops options (const options = args.pop() as
Handlebars.HelperOptions) and return options.fn(this) when the boolean check
passes or options.inverse(this) otherwise for each helper.
In `@packages/gql/codegen/index.ts`:
- Around line 144-148: The module currently calls main() unconditionally which
runs codegen on every import; wrap the invocation in an execution guard so it
only runs when the script is executed directly (e.g. use if (import.meta?.main)
{ main().catch(...) } or fallback to require.main === module for CJS/Bundler
compatibility) so exported functions/classes can be imported without triggering
code generation; update the existing unconditional main() invocation accordingly
(the symbol to change is main()).
In `@packages/gql/codegen/plugins/dart.ts`:
- Around line 805-821: In getOperationReturnType, remove the nullable branch for
the special-case VoidResult so it never returns "void?" — always return "void"
when field.returnType.name === 'VoidResult'; update the logic in the
getOperationReturnType method (and any related places that might append a '?'
for VoidResult) to ensure nullable handling is skipped for VoidResult to avoid
producing invalid Dart "void?" syntax.
- Around line 444-464: In generateRequestPurchaseProps add warnings when schema
lookups fall back to hardcoded types: detect when purchaseByPlatforms or
subsByPlatforms is undefined or when fields 'apple'/'google' are missing and
call a logger (e.g. this.logger.warn) including the missing schema name
(RequestPurchasePropsByPlatforms or RequestSubscriptionPropsByPlatforms) and
which fallback was used (appleType, googleType, appleSubsType, googleSubsType),
so any use of the default
'RequestPurchaseIosProps'/'RequestPurchaseAndroidProps'/'RequestSubscriptionIosProps'/'RequestSubscriptionAndroidProps'
is logged for visibility.
In `@packages/gql/codegen/templates/dart/header.hbs`:
- Around line 1-4: The generated header template header.hbs currently instructs
users to run "npm run generate" which is incorrect for this package; open the
template (header.hbs) and replace the "npm run generate" text with "bun run
generate" so the auto-generated files show the correct command, ensuring the
rest of the header text remains unchanged.
In `@packages/gql/codegen/templates/gdscript/enum.hbs`:
- Around line 1-9: The template emits GDScript doc comments but only prefixes
the first line of multi-line descriptions with "##", so subsequent lines become
code; add a helper (e.g., gd_doc or prefixLinesWithHash) that takes a
description string and returns it with every line prefixed by "## " (preserving
existing newlines), then call that helper wherever {{description}} is used in
packages/gql/codegen/templates/gdscript/enum.hbs (for the enum-level description
and each value description) so that the variables name, values, caseName and
rawValue remain unchanged but the rendered doc comment lines are all correctly
prefixed.
In `@packages/gql/codegen/templates/gdscript/header.hbs`:
- Around line 1-4: Update the header template in header.hbs so the generated
banner uses the repository standard command; replace the "npm run generate"
instruction with "bun run generate" (i.e., edit the AUTO-GENERATED TYPES — DO
NOT EDIT DIRECTLY banner text to instruct "Run `bun run generate` after updating
any *.graphql schema file.") to keep the package/gql instructions consistent.
In `@packages/gql/codegen/templates/gdscript/operation.hbs`:
- Around line 7-10: The template unconditionally emits a header comment for each
field using {{description}}, causing empty "## " lines when descriptions are
absent; update the {{`#each` fields}} block in the gdscript/operation.hbs template
to conditionally render the comment only when description is present (wrap the
existing "## {{description}}" line in an {{`#if` description}} ... {{/if}} block)
while leaving the var {{propertyName}}: Callable output unchanged.
In `@packages/gql/codegen/templates/gdscript/union.hbs`:
- Around line 13-21: The two branches under {{`#if` isNested}} in the gdscript
union template are identical; remove the redundant conditional and render a
single mapping inside the {{`#each` concreteMembers}} loop that returns
{{../name}}.new({{delegateTo}}.from_json(json)) for each "{{typeName}}". Edit
the block containing {{`#each` concreteMembers}} / {{/each}} to drop the {{`#if`
isNested}} / {{else}} / {{/if}} lines and leave only the unified mapping using
{{typeName}}, {{../name}} and {{delegateTo}} (or alternatively implement proper
nested handling by wrapping with {{wrapperName}}.new(...) if you prefer matching
Kotlin/Dart behavior).
In `@packages/gql/codegen/templates/kotlin/enum.hbs`:
- Around line 17-27: The Kotlin enum Handlebars template references a
non-registered helper "(eq)" in the line using "{{`#unless` (eq this
../rawValue)}}", causing render failures; fix by either registering a plain "eq"
helper in the template registration code (where "if_eq" and "unless_eq" are
registered) or update the template to use the existing helper "{{`#unless_eq` this
../rawValue}}". Locate the usage in the enum template's fromJson block (the
"{{rawValue}}"/"{{caseName}}" loop) and either add the "eq" helper to the
helpers list in template-engine.ts or replace the inline call with "{{`#unless_eq`
this ../rawValue}}".
In `@packages/gql/codegen/templates/kotlin/header.hbs`:
- Around line 1-4: Replace the incorrect "npm run generate" instruction in the
auto-generated header template with "bun run generate": update the header.hbs
template line that currently reads "Run `npm run generate` after updating any
*.graphql schema file." to instead read "Run `bun run generate` after updating
any *.graphql schema file." so the banner string in the AUTO-GENERATED TYPES
header reflects the repo's correct command.
In `@packages/gql/codegen/templates/swift/header.hbs`:
- Around line 1-4: Update the auto-generated header in the Swift codegen
template (header.hbs) to use the repository's tooling command: replace the "npm
run generate" text with "bun run generate" so the comment instructs running `bun
run generate` after updating any *.graphql schema file.
In `@packages/gql/CONVENTION.md`:
- Around line 109-118: Update the flow diagram in CONVENTION.md to clarify that
generation emits both the shared GraphQL artifacts and language-specific outputs
by amending the final node; reference the existing symbols Parser
(codegen/core/parser.ts), Transformer → IR (codegen/core/transformer.ts), and
Language Plugins (codegen/plugins/*.ts), and change "Generated Files
(src/generated/*)" to something like "Generated Files (src/generated/*) +
language-specific outputs (from codegen/plugins/*.ts)" so readers know plugins
produce additional output.
♻️ Duplicate comments (3)
packages/gql/codegen/templates/dart/object.hbs (1)
1-11: Same multi-line description risk as in dart/interface.hbs.Apply the same multi-line-safe doc comment approach here (class + field descriptions).
packages/gql/codegen/templates/gdscript/interface.hbs (1)
1-9: Duplicate: multi-line GDScript doc comment handling.Same issue as in gdscript/enum.hbs—ensure every description line is prefixed (or normalized).
packages/gql/codegen/templates/gdscript/object.hbs (1)
1-9: Duplicate: multi-line GDScript doc comment handling.Same concern as gdscript/enum.hbs—multi-line descriptions need per-line prefixing.
🧹 Nitpick comments (21)
packages/gql/codegen/templates/kotlin/object.hbs (1)
1-33: Consider removing trailing commas for broader Kotlin compatibility.If any consumers target Kotlin <1.4, trailing commas in argument lists can fail; guarding with
isLastavoids that without changing behavior on newer compilers.♻️ Suggested template tweak
@@ - {{propertyName}} = {{fromJsonExpr}}, + {{propertyName}} = {{fromJsonExpr}}{{`#unless` isLast}},{{/unless}} @@ - "{{graphqlName}}" to {{toJsonExpr}}, + "{{graphqlName}}" to {{toJsonExpr}}{{`#unless` isLast}},{{/unless}}packages/gql/codegen/templates/kotlin/operation-helpers.hbs (1)
1-1: Minor:// MARK: -is a Swift convention, not idiomatic Kotlin.Kotlin typically uses
// region/// endregionfor code folding or simple// ---- Section ----comments. This is a minor stylistic inconsistency but doesn't affect functionality.♻️ Optional: Use Kotlin-idiomatic section markers
-// MARK: - {{name}} Helpers +// region {{name}} HelpersOr simply:
-// MARK: - {{name}} Helpers +// --- {{name}} Helpers ---packages/gql/codegen/index.ts (2)
136-138: CLI arguments are type-cast without validation.User-provided arguments are cast directly to the union type without checking if they're valid language names. Invalid inputs like
bun run generate pythonwill silently pass tocreatePlugin()and be skipped with a log message, which may confuse users.♻️ Proposed fix: validate CLI arguments
+const SUPPORTED_LANGUAGES = ['swift', 'kotlin', 'dart', 'gdscript'] as const; +type SupportedLanguage = typeof SUPPORTED_LANGUAGES[number]; + async function main() { const args = process.argv.slice(2); - const languages = args.length > 0 - ? args as Array<'swift' | 'kotlin' | 'dart' | 'gdscript'> - : ['swift', 'kotlin', 'dart', 'gdscript']; + const languages: SupportedLanguage[] = args.length > 0 + ? args.filter((arg): arg is SupportedLanguage => + SUPPORTED_LANGUAGES.includes(arg as SupportedLanguage) + ) + : [...SUPPORTED_LANGUAGES]; + + if (args.length > 0 && languages.length === 0) { + console.error(`Invalid languages. Supported: ${SUPPORTED_LANGUAGES.join(', ')}`); + process.exit(1); + } const generator = new CodeGenerator({ languages }); await generator.generate(); }
43-48: Default languages mismatch between constructor and CLI.The
CodeGeneratorconstructor defaults to['swift', 'kotlin'](line 45), while the CLI'smain()defaults to all four languages (line 138). This inconsistency could lead to unexpected behavior when using the class programmatically versus via CLI.♻️ Proposed fix: align defaults or make them explicit
+const ALL_LANGUAGES: Array<'swift' | 'kotlin' | 'dart' | 'gdscript'> = ['swift', 'kotlin', 'dart', 'gdscript']; + export class CodeGenerator { private config: GenerateConfig; private schema: IRSchema | null = null; constructor(config: GenerateConfig = {}) { this.config = { - languages: config.languages ?? ['swift', 'kotlin'], + languages: config.languages ?? ALL_LANGUAGES, outputDir: config.outputDir ?? resolve(__dirname, '../src/generated'), verbose: config.verbose ?? true, }; }packages/gql/codegen/core/parser.ts (2)
81-85: Missing error handling for schema file reads.
readFileSyncwill throw if a schema file doesn't exist or is unreadable. Consider wrapping with error handling to provide clearer diagnostics about which file failed.♻️ Proposed fix: add error context
// Load all SDL files for (const schemaPath of this.schemaPaths) { - const content = readFileSync(schemaPath, 'utf8'); - sdlContents.set(schemaPath, content); + try { + const content = readFileSync(schemaPath, 'utf8'); + sdlContents.set(schemaPath, content); + } catch (error) { + throw new Error(`Failed to read schema file: ${schemaPath}`, { cause: error }); + } }
148-161: Field matching logic may skip valid fields after comments.When
expectFutureFieldis true and we encounter a non-matching line (not a field, not empty, not a comment), the flag is reset at line 160. However, thecontinueat line 158 for empty/comment lines means if there's an intervening blank line followed by a non-field line, the marker is lost. This logic is correct but could be fragile if SDL formatting varies.Consider documenting the expected SDL format for
# Futuremarkers in CONVENTION.md to ensure consistent usage.packages/gql/codegen/plugins/template-plugin.ts (3)
208-218: Property namecamelCaseNameproduces PascalCase output.The
camelCaseNameproperty (line 213) capitalizes the first character, which produces PascalCase (e.g.,None→None), not camelCase (which would benone). This naming inconsistency could confuse template authors.♻️ Proposed fix: rename or adjust casing
Either rename to match the actual casing:
- camelCaseName: value.name.charAt(0).toUpperCase() + value.name.slice(1), + pascalCaseName: value.name.charAt(0).toUpperCase() + value.name.slice(1),Or produce actual camelCase:
- camelCaseName: value.name.charAt(0).toUpperCase() + value.name.slice(1), + camelCaseName: value.name.charAt(0).toLowerCase() + value.name.slice(1),
182-189: Silent failure on missing templates may hide configuration issues.The empty
catchblock silently swallows all errors, including unexpected failures like permission issues. Consider logging a debug message or distinguishing between "file not found" (expected) and other errors (unexpected).♻️ Proposed fix: handle errors more granularly
for (const name of templateFiles) { try { const content = readFileSync(join(templatePath, `${name}.hbs`), 'utf-8'); this.templates.set(name, this.handlebars.compile(content)); - } catch { + } catch (error) { // Template not found - plugin will use code generation instead + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn(`[${this.templateDir}] Unexpected error loading template ${name}:`, error); + } } }
308-309: Unconventional import placement at end of file.The
IREnumValueimport is placed at the end of the file rather than with other imports at the top. This appears to be a workaround, but it reduces code readability.♻️ Proposed fix: move import to top
import type { IRSchema, IREnum, IRInterface, IRObject, IRInput, IRUnion, IROperation, IRType, IRField, IROperationField, + IREnumValue, } from '../core/types.js'; -// Re-export IREnumValue for context builders -import type { IREnumValue } from '../core/types.js';packages/gql/codegen/templates/kotlin/input.hbs (1)
25-29: Trailing commas on all constructor parameters regardless of position.Lines 26-28 emit a trailing comma after every parameter unconditionally. While Kotlin 1.4+ allows trailing commas, this is inconsistent with lines 13 which uses
{{#unlessisLast}},{{/unless}}for the primary constructor. Consider using consistent comma handling throughout.♻️ Proposed fix: use consistent comma handling
return {{name}}( {{`#each` fields}} - {{propertyName}} = {{propertyName}}, + {{propertyName}} = {{propertyName}}{{`#unless` isLast}},{{/unless}} {{/each}} )packages/gql/codegen/core/transformer.ts (2)
214-239: Type assertion assumes named type hasnameproperty.Line 216 casts
graphqlTypeto{ name: string }without checking that it's actually a named type. While the control flow ensures we've passed theGraphQLNonNullandGraphQLListchecks, this could be made more explicit for type safety.♻️ Proposed fix: use GraphQL's getNamedType or type guards
+import { getNamedType, isNamedType } from 'graphql'; // Named type - const typeName = (graphqlType as { name: string }).name; + if (!isNamedType(graphqlType)) { + throw new Error(`Expected named type but got ${graphqlType}`); + } + const typeName = graphqlType.name; let kind: IRType['kind'] = 'object';Or use the imported
getNamedType:// Named type - const typeName = (graphqlType as { name: string }).name; + const namedType = getNamedType(graphqlType); + const typeName = namedType.name;
254-260: Legacy alias lookup iterates all entries for each enum value.The
Object.entries(ERROR_CODE_LEGACY_ALIASES).filter(...)runs for every ErrorCode enum value, resulting in O(n×m) complexity. For small enums this is fine, but consider pre-computing a reverse lookup map if ErrorCode grows large.This is a minor optimization opportunity for future consideration—current implementation is correct and likely sufficient for the expected enum size.
packages/gql/codegen/plugins/kotlin.ts (3)
45-47: Unknown scalar types silently default toString.When
mapScalar()encounters an unknown scalar name not inGRAPHQL_TO_KOTLIN, it returns'String'without warning. This could mask schema issues where custom scalars aren't properly handled.♻️ Proposed fix: log warning for unknown scalars
mapScalar(name: string): string { - return GRAPHQL_TO_KOTLIN[name] ?? 'String'; + const mapped = GRAPHQL_TO_KOTLIN[name]; + if (!mapped) { + console.warn(`[kotlin] Unknown scalar type "${name}", defaulting to String`); + return 'String'; + } + return mapped; }
771-782: Enum deserialization falls back to first value for non-nullable required enums.When a non-nullable enum field has no "Empty" value and receives invalid/missing JSON, it falls back to the first enum value (line 777-780). This silent fallback could mask data issues. Consider whether throwing an exception would be more appropriate for required fields with invalid data.
The current behavior matches typical Kotlin defensive coding patterns, but document this fallback behavior in the generated code comments or CONVENTION.md so consumers are aware.
334-415: Significant code duplication betweengenerateInputandgenerateStandardInput.Both methods share nearly identical logic for generating Kotlin data classes with
fromJson/toJson. The main difference is thatgenerateInputsorts fields whilegenerateStandardInputuses original order.♻️ Proposed fix: extract common logic
+ private generateInputBody( + irInput: IRInput, + fields: IRField[], + ): void { + this.generateDocComment(irInput.description); + this.emit(`public data class ${irInput.name}(`); + + fields.forEach((field, index) => { + this.generateDocComment(field.description, ' '); + const propertyType = this.getPropertyType(field.type); + const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); + const suffix = index === fields.length - 1 ? '' : ','; + const defaultValue = field.type.nullable ? ' = null' : ''; + this.emit(` val ${propertyName}: ${propertyType}${defaultValue}${suffix}`); + }); + + // ... rest of fromJson/toJson generation + } generateInput(irInput: IRInput): void { if (irInput.isCustomType) { this.generateCustomInput(irInput); return; } - // Sort fields alphabetically for Kotlin const sortedFields = [...irInput.fields].sort((a, b) => a.name.localeCompare(b.name)); - // ... duplicated logic + this.generateInputBody(irInput, sortedFields); } private generateStandardInput(irInput: IRInput): void { - // ... duplicated logic + this.generateInputBody(irInput, irInput.fields); }packages/gql/codegen/plugins/gdscript.ts (2)
21-27: Unused import:toKebabCaseThe
toKebabCaseutility is imported but not used anywhere in this file.🧹 Remove unused import
import { GDSCRIPT_KEYWORDS, GRAPHQL_TO_GDSCRIPT, toSnakeCase, toConstantCase, - toKebabCase, } from '../core/utils.js';
310-341: Consider extracting shared logic forfrom_dictfield generation.
generateFromDictField(lines 310-341) andgenerateInputFromDictField(lines 427-458) are nearly identical. Consolidating them would reduce maintenance burden and ensure consistent behavior.♻️ Suggested approach
Extract a shared private method that both can delegate to:
private generateFromDictFieldCore(field: IRField, fieldName: string): void { // shared implementation } private generateFromDictField(field: IRField, fieldName: string): void { this.generateFromDictFieldCore(field, fieldName); } private generateInputFromDictField(field: IRField, fieldName: string): void { this.generateFromDictFieldCore(field, fieldName); }Also applies to: 427-458
packages/gql/codegen/plugins/dart.ts (1)
21-27: Unused import:toKebabCaseThe
toKebabCaseutility is imported but not referenced in this file.🧹 Remove unused import
import { DART_KEYWORDS, GRAPHQL_TO_DART, toPascalCasePreserveIOS, - toKebabCase, PLATFORM_TYPE_DEFAULTS, } from '../core/utils.js';packages/gql/codegen/core/utils.ts (1)
527-530: Inconsistent Dart import ingenerateFileHeader.The
generateFileHeaderfunction addsdart:convertimport for Dart (line 528), but the actual Dart plugin (dart.ts, line 165) generatesdart:asyncimport instead. This utility function appears unused, but if it were used, it would generate an incorrect import.Either update this to match the plugin's actual needs:
case 'dart': - header.push("import 'dart:convert';", ''); + header.push("import 'dart:async';", ''); break;Or remove this dead code path if the function isn't being used.
packages/gql/codegen/plugins/swift.ts (1)
20-28: Imported constant not used; logic duplicated locally.
ERROR_CODE_LEGACY_ALIASESis imported (line 27) but not used. Instead, the same mapping is redefined locally at lines 104-107. Consider using the imported constant for consistency and maintainability.♻️ Use imported constant
// Add custom initializer for ErrorCode to handle legacy aliases if (irEnum.isErrorCode) { - // Legacy aliases: old error codes that map to new ones - const legacyAliases: Record<string, string> = { - 'receipt-failed': 'purchaseVerificationFailed', - 'ReceiptFailed': 'purchaseVerificationFailed', - }; + // Use imported legacy aliases for consistency + const legacyAliases = ERROR_CODE_LEGACY_ALIASES;Also applies to: 101-107
packages/gql/codegen/core/template-engine.ts (1)
7-9: Unused import:IROperationField
IROperationFieldis imported but not used anywhere in this file.🧹 Remove unused import
-import type { IRType, IRField, IREnum, IREnumValue, IROperationField } from './types.js'; +import type { IRType, IRField, IREnum, IREnumValue } from './types.js';
- parser.ts: Use strict regex for # Future marker matching - template-engine.ts: Use regular functions for correct this binding - template-engine.ts: Add 'eq' and 'gd_doc' helpers - index.ts: Guard main() execution for library usage - dart.ts: Add logging for fallback type detection - dart.ts: Remove void? handling (invalid Dart syntax) - header.hbs: Update npm -> bun command in all templates - gdscript/enum.hbs: Use gd_doc helper for multi-line comments - gdscript/operation.hbs: Add conditional for field description - gdscript/union.hbs: Remove redundant isNested conditional - CONVENTION.md: Clarify sync step in flow diagram Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/gql/codegen/plugins/dart.ts`:
- Around line 157-161: In generateHeader(), update the generated header text so
the guidance line uses "bun run generate" instead of "npm run generate"; locate
the generateHeader() method in packages/gql/codegen/plugins/dart.ts and replace
the string in the emit call that currently reads "Run `npm run generate` after
updating any *.graphql schema file." with the corrected "Run `bun run generate`
after updating any *.graphql schema file." to keep the auto-generated header
accurate.
♻️ Duplicate comments (1)
packages/gql/CONVENTION.md (1)
109-118: Clarify language-specific outputs in the flow diagram.The flow stops at
src/generated/*, but the plugins also emit language-specific outputs (e.g.,dist/*). Consider reflecting that in the final node to avoid confusion.💡 Suggested tweak
- Generated Files (src/generated/*) + Generated Files (src/generated/* + language-specific dist/* outputs)Based on learnings, this diagram should reflect all generated targets.
🧹 Nitpick comments (2)
packages/gql/codegen/core/template-engine.ts (1)
268-315: Add JSDoc to exported context builders.The exported builder functions don’t have JSDoc yet. Adding brief docs keeps the public API consistent with guidelines.
💡 Suggested docs
-export function buildEnumContext( +/** + * Build template context for an enum IR node. + */ +export function buildEnumContext( irEnum: IREnum, config: ContextBuilderConfig ): EnumContext {-export function buildFieldContext( +/** + * Build template context for a single field. + */ +export function buildFieldContext( field: IRField, config: ContextBuilderConfig, isLast: boolean ): FieldContext {-export function buildFieldsContext( +/** + * Build template contexts for a list of fields. + */ +export function buildFieldsContext( fields: IRField[], config: ContextBuilderConfig, sort: boolean = false ): FieldContext[] {As per coding guidelines, public exports should include JSDoc.
packages/gql/codegen/core/parser.ts (1)
116-161: Consider skipping block descriptions while waiting for a field after# Futuremarker.The parser's handling of
# Futuremarkers is fragile: whenexpectFutureFieldis set, block descriptions (""") are treated as "unexpected" lines and clear the marker. While this pattern doesn't currently occur in the schema files, it could realistically be introduced. If a field is documented with a block string between# Futureand its definition, the async marker would be lost, violating the schema convention that all async operations must have# Future.Suggested fix (skip block descriptions while awaiting field)
- let expectFutureField = false; + let expectFutureField = false; + let inBlockDescription = false; ... - // Handle field after # Future marker + // Track block string descriptions (""") while waiting for a future field + const tripleQuoteCount = (trimmed.match(/"""/g) || []).length; + if (tripleQuoteCount > 0) { + if (tripleQuoteCount % 2 === 1) inBlockDescription = !inBlockDescription; + if (expectFutureField) { + continue; + } + } + if (expectFutureField && inBlockDescription) { + continue; + } + + // Handle field after # Future marker if (expectFutureField && currentTypeName) { const fieldMatch = trimmed.match(/^([A-Za-z0-9_]+)\s*[:(]/);
- dart.ts: npm run generate → bun run generate - kotlin.ts: npm run generate → bun run generate - swift.ts: npm run generate → bun run generate - gdscript.ts: npm run generate:gdscript → bun run generate Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add table showing correct vs incorrect format - Add multiple examples of correct replies - Make the rule more prominent with CRITICAL label Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.claude/commands/review-pr.md:
- Around line 38-58: Add a blank line after the "Commit Hash Formatting"
heading, insert a blank line before each code block that starts with the
examples containing "Fixed in f3b5fec." and "Fixed in abc1234 along with other
review items.", and add a language specifier (e.g., text or markdown) to both
code fences so markdownlint-cli2 passes; update the two code fences shown in the
examples to use ```text (or ```markdown) and ensure the surrounding blank lines
are present.
In `@packages/gql/codegen/plugins/gdscript.ts`:
- Around line 98-106: The sets enumNames, objectNames, inputNames, and
unionNames are not cleared when generate(schema: IRSchema) runs, causing stale
entries if the plugin instance is reused; at the start of the generate method
(generate(schema)) call clear() on each set (enumNames.clear(),
objectNames.clear(), inputNames.clear(), unionNames.clear()) before repopulating
them so isObjectOrInput() and related checks only see current schema types.
- Around line 522-528: The generated GDScript can produce reserved identifiers
because argSnakeName is set from toSnakeCase(arg.name) without escaping; update
the args emission loop (where field.args is iterated and var ${argSnakeName}:
${argType} is emitted) to run arg names through escapeKeyword() (i.e., const
argSnakeName = escapeKeyword(toSnakeCase(arg.name))) so reserved words like
"class" or "pass" are renamed, and apply the same escapeKeyword(...)
transformation wherever API helper code constructs or references those Args
names so generation and helpers remain consistent (check the functions that
build API helper signatures/usages that reference arg names).
- Around line 586-599: The current loop in generateApiHelpers handling
field.args only checks isObjectOrInputByName(arg.type.name), which misses
list-typed args because arg.type.name is undefined for lists; update the logic
in the field.args loop to detect list types (e.g., when arg.type.kind or
arg.type.ofType indicates a LIST) and, for list-of-object/input, iterate over
the list to call to_dict() on each item (or map items to .to_dict() when
has_method("to_dict")) before assigning to args["<arg.name>"]; keep the existing
single-object branch (using has_method("to_dict") on argSnakeName) and the
primitive branch, but add a new branch for list-of-objects that produces an
array of dicts so JSON serialization works correctly.
In `@packages/gql/codegen/plugins/kotlin.ts`:
- Around line 734-741: The current logic in buildFromJsonExpression for list
types uses mapNotNull (via mapFn) to drop null elements for non-null element
types, which silently alters list length; change this to fail fast on schema
violations by avoiding mapNotNull and instead explicitly check for null elements
and throw or propagate an error when an element is null for a non‑nullable
element type. Locate the list handling branch (type.kind === 'list') in
buildFromJsonExpression, remove the use of mapNotNull for non‑nullable element
types, and replace it with a mapping implementation that preserves list length
and raises a clear exception (or returns an error) if any mapped element is
null; ensure existing nullable handling (type.nullable or forNullableFromJson)
remains intact and that sourceExpr and element generation (element =
this.buildFromJsonExpression(...)) are reused.
- Around line 650-705: The code currently always prefixes Kotlin functions and
typealiases with "suspend"; update generateOperationInterface and
generateOperationHelpers to only emit the "suspend" modifier when the IR field
has field.isFuture === true. In generateOperationInterface (function name:
generateOperationInterface) gate the emitted signature `suspend fun ...` so it
becomes `${field.isFuture ? 'suspend ' : ''}fun ...` (i.e., only prepend
"suspend" when field.isFuture), and in generateOperationHelpers (function name:
generateOperationHelpers) do the same for handler aliases so `public typealias
... = suspend (...) -> ${returnType}` is emitted only when field.isFuture is
true and otherwise emit `public typealias ... = (...) -> ${returnType}` (apply
both the zero-arg and multi-arg branches).
🧹 Nitpick comments (8)
packages/gql/codegen/plugins/swift.ts (5)
24-24: Unused import:toKebabCase.The
toKebabCasefunction is imported but not used anywhere in this file.Proposed fix
import { SWIFT_KEYWORDS, GRAPHQL_TO_SWIFT, toLowerCamelCase, - toKebabCase, capitalize, PLATFORM_TYPE_DEFAULTS, ERROR_CODE_LEGACY_ALIASES, } from '../core/utils.js';
37-39: Redundant constructor.The constructor only calls
super(config)with no additional logic. TypeScript will automatically generate this when the class has a parent constructor with matching signature.Proposed fix
readonly keywords = SWIFT_KEYWORDS; private schema!: IRSchema; - - constructor(config: CodegenPluginConfig) { - super(config); - } // ============================================================================
292-305: Consider documenting the dependency onPurchasetype.The
PurchaseInputtypealias references aPurchasetype that must exist in the generated output. While this likely works correctly given the schema, a brief comment documenting this dependency would improve maintainability.
27-27: Remove unused importsERROR_CODE_LEGACY_ALIASESandtoKebabCase; replace hardcoded aliases with the imported constant.
ERROR_CODE_LEGACY_ALIASESis imported from../core/utils.jsbut never used—the legacy aliases are hardcoded at lines 104-107 instead. The hardcoded values match the constant exactly, creating unnecessary duplication that risks drift if the central definition changes. Additionally,toKebabCaseis imported but unused. Use the constant or remove both from the import list.Proposed fix
Replace the hardcoded aliases with the imported constant:
import { SWIFT_KEYWORDS, GRAPHQL_TO_SWIFT, toLowerCamelCase, capitalize, PLATFORM_TYPE_DEFAULTS, ERROR_CODE_LEGACY_ALIASES, } from '../core/utils.js'; -const legacyAliases: Record<string, string> = { - 'receipt-failed': 'purchaseVerificationFailed', - 'ReceiptFailed': 'purchaseVerificationFailed', -}; +const legacyAliases = ERROR_CODE_LEGACY_ALIASES;Or remove both unused imports if intentional to keep hardcoded values.
510-514:generateOperationimplementation doesn't match its comment and is bypassed by the overriddengenerate()method.While this method is required by the abstract base class interface, SwiftGenerator's
generate()override (line 521) bypasses it entirely and callsgenerateOperationProtocol(line 572) andgenerateOperationHelpers(line 580) directly. The comment on line 511-512 is misleading—the method only generates the protocol, not both protocol and helpers.Either use the inherited base pattern (call
super.generate()or letgenerateOperationhandle both protocol and helper generation), or update the comment to reflect that this method is not actually used by the Swift generator's flow.packages/gql/codegen/plugins/gdscript.ts (1)
29-33: Add JSDoc for the exported plugin API.The exported class and its public methods don’t have JSDoc, but our TS guidelines require JSDoc for public functions/exported APIs. Please add concise docs on the class and key public methods (e.g.,
generate,mapType,generateEnum,generateObject,generateInput,generateOperation).As per coding guidelines, ...
packages/gql/codegen/plugins/kotlin.ts (2)
436-502: Keep custom input field ordering deterministic
generateInputsorts fields, butgenerateStandardInputuses the original order. That makes custom inputs inconsistent with the rest of the Kotlin output and can introduce churn if IR ordering changes.♻️ Suggested refactor (align ordering)
- irInput.fields.forEach((field, index) => { + const sortedFields = [...irInput.fields].sort((a, b) => a.name.localeCompare(b.name)); + sortedFields.forEach((field, index) => { this.generateDocComment(field.description, ' '); const propertyType = this.getPropertyType(field.type); const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); - const suffix = index === irInput.fields.length - 1 ? '' : ','; + const suffix = index === sortedFields.length - 1 ? '' : ','; const defaultValue = field.type.nullable ? ' = null' : ''; this.emit(` val ${propertyName}: ${propertyType}${defaultValue}${suffix}`); }); @@ - const hasRequiredFields = irInput.fields.some((f) => !f.type.nullable); + const hasRequiredFields = sortedFields.some((f) => !f.type.nullable); @@ - for (const field of irInput.fields) { + for (const field of sortedFields) { const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); const expression = this.buildFromJsonExpression(field.type, `json["${field.name}"]`, false, true); this.emit(` val ${propertyName} = ${expression}`); } @@ - const requiredFields = irInput.fields.filter( + const requiredFields = sortedFields.filter( (f) => !f.type.nullable && f.type.kind !== 'enum' ); @@ - for (const field of irInput.fields) { + for (const field of sortedFields) { const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); this.emit(` ${propertyName} = ${propertyName},`); } @@ - for (const field of irInput.fields) { + for (const field of sortedFields) { const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); const expression = this.buildFromJsonExpression(field.type, `json["${field.name}"]`); this.emit(` ${propertyName} = ${expression},`); } @@ - for (const field of irInput.fields) { + for (const field of sortedFields) { const propertyName = this.escapeKeyword(this.fieldNameCase(field.name)); const expression = this.buildToJsonExpression(field.type, propertyName); this.emit(` "${field.name}" to ${expression},`); }
30-149: Add JSDoc for public APIs inKotlinPluginThe exported class exposes multiple public methods without JSDoc. Please add brief JSDoc comments to the public API surface (e.g.,
mapScalar,mapType,generate, etc.). As per coding guidelines, add JSDoc comments for public functions and exported APIs.
| if (type.kind === 'list') { | ||
| const element = this.buildFromJsonExpression(type.elementType!, 'it', true, forNullableFromJson); | ||
| const mapFn = type.elementType!.nullable ? 'map' : 'mapNotNull'; | ||
| if (type.nullable || forNullableFromJson) { | ||
| return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} }`; | ||
| } | ||
| return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} } ?: emptyList()`; | ||
| } |
There was a problem hiding this comment.
Avoid silently dropping null list elements
mapNotNull removes nulls for non‑null list element types, which can hide invalid data and change list length. Prefer failing fast for schema‑violating nulls.
🐛 Suggested fix (preserve list length and fail on nulls)
- const mapFn = type.elementType!.nullable ? 'map' : 'mapNotNull';
- if (type.nullable || forNullableFromJson) {
- return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} }`;
- }
- return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} } ?: emptyList()`;
+ const elementExpr = type.elementType!.nullable
+ ? element
+ : `requireNotNull(${element}) { "Null element in list ${sourceExpr}" }`;
+ if (type.nullable || forNullableFromJson) {
+ return `(${sourceExpr} as? List<*>)?.map { ${elementExpr} }`;
+ }
+ return `(${sourceExpr} as? List<*>)?.map { ${elementExpr} } ?: emptyList()`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (type.kind === 'list') { | |
| const element = this.buildFromJsonExpression(type.elementType!, 'it', true, forNullableFromJson); | |
| const mapFn = type.elementType!.nullable ? 'map' : 'mapNotNull'; | |
| if (type.nullable || forNullableFromJson) { | |
| return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} }`; | |
| } | |
| return `(${sourceExpr} as? List<*>)?.${mapFn} { ${element} } ?: emptyList()`; | |
| } | |
| if (type.kind === 'list') { | |
| const element = this.buildFromJsonExpression(type.elementType!, 'it', true, forNullableFromJson); | |
| const elementExpr = type.elementType!.nullable | |
| ? element | |
| : `requireNotNull(${element}) { "Null element in list ${sourceExpr}" }`; | |
| if (type.nullable || forNullableFromJson) { | |
| return `(${sourceExpr} as? List<*>)?.map { ${elementExpr} }`; | |
| } | |
| return `(${sourceExpr} as? List<*>)?.map { ${elementExpr} } ?: emptyList()`; | |
| } |
🤖 Prompt for AI Agents
In `@packages/gql/codegen/plugins/kotlin.ts` around lines 734 - 741, The current
logic in buildFromJsonExpression for list types uses mapNotNull (via mapFn) to
drop null elements for non-null element types, which silently alters list
length; change this to fail fast on schema violations by avoiding mapNotNull and
instead explicitly check for null elements and throw or propagate an error when
an element is null for a non‑nullable element type. Locate the list handling
branch (type.kind === 'list') in buildFromJsonExpression, remove the use of
mapNotNull for non‑nullable element types, and replace it with a mapping
implementation that preserves list length and raises a clear exception (or
returns an error) if any mapped element is null; ensure existing nullable
handling (type.nullable or forNullableFromJson) remains intact and that
sourceExpr and element generation (element = this.buildFromJsonExpression(...))
are reused.
There was a problem hiding this comment.
Thank you for the suggestion. However, changing mapNotNull to requireNotNull would alter the runtime behavior of the generated code.
This PR's goal is to maintain 100% identical output to the previous generator scripts, which used mapNotNull. Changing to fail-fast behavior would be a breaking change for existing consumers who may rely on the current null-filtering behavior.
This would be a good enhancement for a follow-up PR with proper versioning and documentation of the behavioral change.
There was a problem hiding this comment.
@hyochan, thank you for the clarification! You're absolutely right—maintaining byte-for-byte compatibility with the existing generator output is the correct approach for this refactoring PR.
I understand that the current mapNotNull behavior is intentional and that changing to fail-fast semantics would be a breaking change for consumers who may depend on the null-filtering behavior. Deferring this enhancement to a separate PR with proper versioning makes perfect sense.
Thanks for keeping the refactor focused on architectural improvements without introducing behavioral changes. 👍
🧠 Learnings used
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: Applies to packages/google/** : Run ./scripts/generate-types.sh to regenerate Kotlin types when updating openiap-gql version in openiap-versions.json
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: Applies to packages/google/src/main/Types.kt : DO NOT edit generated files: packages/google/src/main/Types.kt is auto-generated from openiap-gql and must be regenerated via ./scripts/generate-types.sh
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: Applies to packages/google/** : Compile Android code with ./gradlew :openiap:compileDebugKotlin to verify after type generation
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: For cross-package changes, review CONVENTION.md files for all affected packages (packages/gql/CONVENTION.md, packages/google/CONVENTION.md, packages/apple/CONVENTION.md)
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: Applies to packages/gql/** : Generate GraphQL types by running 'bun run generate' in packages/gql, which outputs TypeScript (src/generated/types.ts), Swift (dist/swift/Types.swift), Kotlin (dist/kotlin/Types.kt), and Dart (dist/dart/types.dart)
Learnt from: CR
Repo: hyodotdev/openiap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T16:22:04.517Z
Learning: Applies to packages/apple/** : Update Apple types by editing openiap-versions.json (gql field), running ./scripts/generate-types.sh, and verifying with 'swift test'
- review-pr.md: Fix markdown formatting (blank lines, code block language) - gdscript.ts: Clear type caches at start of generate() for plugin reuse - gdscript.ts: Escape GDScript keywords in generated argument names - gdscript.ts: Handle list-of-object serialization in API helpers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Architecture
Changes
New Plugin System (
codegen/plugins/)Deleted Scripts (3,687 lines removed)
generate-swift-types.mjs(831 lines)generate-kotlin-types.mjs(939 lines)generate-dart-types.mjs(1,105 lines)generate-gdscript-types.mjs(812 lines)Documentation Updates
CONTRIBUTING.md- Added IR architecture documentationCLAUDE.md- Added code generation system sectionpackages/gql/CONVENTION.md- Added code generation architecture sectionknowledge/internal/04-platform-packages.md- Comprehensive GQL sectionTest plan
bun run generatecompletes successfullypackages/appleandpackages/google🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.