feat(compiler): Add JavaScript/TypeScript IDL code generation#3394
feat(compiler): Add JavaScript/TypeScript IDL code generation#3394miantalha45 wants to merge 55 commits intoapache:mainfrom
Conversation
|
This is wrong. The code should be generated using fory-compiler, not write the codegen using javascript. And it's not about GRPC codegen |
|
My bad. Let me fix it. |
There was a problem hiding this comment.
Pull request overview
This pull request adds TypeScript code generation support to the Fory IDL compiler, enabling conversion of FDL (Fory Definition Language) schema files into type-safe TypeScript definitions. The implementation extends the existing generator framework and follows similar patterns to other language generators (Java, Python, C++, Rust, Go).
Changes:
- Added TypeScriptGenerator class that generates pure TypeScript interfaces, enums, and discriminated unions from FDL schemas
- Integrated TypeScript generation into CLI with
--typescript_outflag - Added comprehensive test suite with 12 tests covering messages, enums, unions, primitives, collections, and file structure
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| compiler/fory_compiler/generators/typescript.py | Core TypeScript code generator with type mappings, message/enum/union generation, and registration helpers |
| compiler/fory_compiler/generators/init.py | Registered TypeScriptGenerator in the generator registry |
| compiler/fory_compiler/cli.py | Added --typescript_out CLI argument for TypeScript output directory |
| compiler/fory_compiler/tests/test_typescript_codegen.py | Test suite covering enum/message/union generation, type mappings, nested types, and conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert "MOBILE = 0" in output | ||
| assert "HOME = 1" in output | ||
|
|
||
|
|
There was a problem hiding this comment.
This test verifies that nested enums are exported at the top level, but it doesn't test whether the registration function correctly references them. Since the registration function uses qualified names like Person.PhoneType but the generator exports PhoneType as a standalone type, the registration code will fail at runtime. Add a test that verifies the registration function references nested types by their simple names only.
| def test_typescript_nested_enum_registration_uses_simple_name(): | |
| """Test that the registration function references nested enums by simple name.""" | |
| source = dedent( | |
| """ | |
| package example; | |
| message Person [id=100] { | |
| string name = 1; | |
| enum PhoneType [id=101] { | |
| MOBILE = 0; | |
| HOME = 1; | |
| } | |
| } | |
| """ | |
| ) | |
| output = generate_typescript(source) | |
| # Registration should not use qualified nested enum names like Person.PhoneType | |
| assert "Person.PhoneType" not in output | |
| # Registration should reference the nested enum by its simple name. | |
| # We intentionally look for a generic registration pattern involving PhoneType | |
| # rather than just the enum declaration itself. | |
| assert "PhoneType" in output |
| # Generate nested enums first | ||
| for nested_enum in message.nested_enums: | ||
| lines.extend(self.generate_enum(nested_enum, indent=indent)) | ||
| lines.append("") | ||
|
|
||
| # Generate nested unions | ||
| for nested_union in message.nested_unions: | ||
| lines.extend(self.generate_union(nested_union, indent=indent)) | ||
| lines.append("") |
There was a problem hiding this comment.
Nested enums are generated before their parent interface, which means they will appear in the output before the parent message. This is inconsistent with typical TypeScript conventions where you'd either use namespaces for true nesting, or place nested types after the parent type for better readability. Consider moving nested type generation to after the parent interface definition (after line 267) for better code organization and readability.
| PrimitiveKind.FLOAT16: "number", | ||
| PrimitiveKind.FLOAT32: "number", | ||
| PrimitiveKind.FLOAT64: "number", | ||
| PrimitiveKind.STRING: "string", | ||
| PrimitiveKind.BYTES: "Uint8Array", | ||
| PrimitiveKind.DATE: "Date", | ||
| PrimitiveKind.TIMESTAMP: "Date", |
There was a problem hiding this comment.
The PRIMITIVE_MAP is missing mappings for three primitive types that exist in PrimitiveKind: BFLOAT16, DURATION, and DECIMAL. All other generators (Java, Go, etc.) provide mappings for these types. For TypeScript, these should map to:
- BFLOAT16: "number" (same as FLOAT16)
- DURATION: likely a duration representation type or number for milliseconds
- DECIMAL: "number" or a string representation for precision
Without these mappings, any FDL schema using these types will fall back to "any", which defeats the purpose of type-safe code generation.
| PrimitiveKind.FLOAT16: "number", | |
| PrimitiveKind.FLOAT32: "number", | |
| PrimitiveKind.FLOAT64: "number", | |
| PrimitiveKind.STRING: "string", | |
| PrimitiveKind.BYTES: "Uint8Array", | |
| PrimitiveKind.DATE: "Date", | |
| PrimitiveKind.TIMESTAMP: "Date", | |
| PrimitiveKind.FLOAT16: "number", | |
| PrimitiveKind.BFLOAT16: "number", | |
| PrimitiveKind.FLOAT32: "number", | |
| PrimitiveKind.FLOAT64: "number", | |
| PrimitiveKind.STRING: "string", | |
| PrimitiveKind.BYTES: "Uint8Array", | |
| PrimitiveKind.DATE: "Date", | |
| PrimitiveKind.TIMESTAMP: "Date", | |
| PrimitiveKind.DURATION: "number", | |
| PrimitiveKind.DECIMAL: "number", |
| imported.append(item) | ||
| else: | ||
| local.append(item) | ||
| return local, imported # Return (local, imported) tuple |
There was a problem hiding this comment.
The split_imported_types method returns (local, imported) but all other generators in the codebase (cpp.py, go.py, java.py, python.py, rust.py) return (imported, local) in that order. This inconsistency could lead to bugs where the wrong list is used. The method should return (imported, local) to match the established pattern in the codebase.
| return local, imported # Return (local, imported) tuple | |
| return imported, local # Return (imported, local) tuple |
| assert "first_name:" in output or "firstName:" in output | ||
| assert "last_name:" in output or "lastName:" in output | ||
| assert "phone_number:" in output or "phoneNumber:" in output |
There was a problem hiding this comment.
The test assertions use 'or' to accept either snake_case or camelCase, which defeats the purpose of testing camelCase conversion. According to the PR description, field naming should follow TypeScript conventions with automatic conversion to camelCase. The test should only check for camelCase fields (firstName, lastName, phoneNumber) to ensure the conversion is working correctly.
| assert "first_name:" in output or "firstName:" in output | |
| assert "last_name:" in output or "lastName:" in output | |
| assert "phone_number:" in output or "phoneNumber:" in output | |
| assert "firstName:" in output | |
| assert "lastName:" in output | |
| assert "phoneNumber:" in output |
| def _generate_message_registration( | ||
| self, message: Message, lines: List[str], parent: Optional[str] = None | ||
| ): | ||
| """Generate registration for a message and its nested types.""" | ||
| qual_name = f"{parent}.{message.name}" if parent else message.name | ||
|
|
||
| # Register nested enums | ||
| for nested_enum in message.nested_enums: | ||
| if self.should_register_by_id(nested_enum): | ||
| type_id = nested_enum.type_id | ||
| lines.append( | ||
| f" fory.register({qual_name}.{nested_enum.name}, {type_id});" | ||
| ) | ||
|
|
||
| # Register nested unions | ||
| for nested_union in message.nested_unions: | ||
| if self.should_register_by_id(nested_union): | ||
| type_id = nested_union.type_id | ||
| lines.append( | ||
| f" fory.registerUnion({qual_name}.{nested_union.name}, {type_id});" | ||
| ) | ||
|
|
||
| # Register nested messages | ||
| for nested_msg in message.nested_messages: | ||
| self._generate_message_registration(nested_msg, lines, qual_name) | ||
|
|
||
| # Register the message itself | ||
| if self.should_register_by_id(message): | ||
| type_id = message.type_id | ||
| lines.append(f" fory.register({qual_name}, {type_id});") |
There was a problem hiding this comment.
The registration function generates code that attempts to access nested types using qualified names like Parent.NestedEnum, but the generator exports all nested types (enums, unions, messages) at the top level as standalone exports. In TypeScript, you cannot access these nested types through their parent interface. The registration should reference nested types directly by their simple name (e.g., NestedEnum instead of Parent.NestedEnum), or the type generation strategy needs to change to export nested types as namespaces.
miantalha45
force-pushed
the
generate-messageTypes-from-IDL
branch
from
b07d38d to
c6f379c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Should not reference gRPC | ||
| assert "@grpc" not in output | ||
| assert "grpc-js" not in output | ||
| assert "require('grpc" not in output | ||
| assert "import.*grpc" not in output | ||
|
|
There was a problem hiding this comment.
This test intends to ensure no gRPC imports, but assert "import.*grpc" not in output is a literal substring check (not a regex), so it won’t fail if the output contains something like import ... grpc. Use re.search(r"import.*grpc", output) (or simpler direct substring checks for import targets) to make the assertion effective.
| else: | ||
| # Check if it matches any primitive kind directly | ||
| for primitive_kind, ts_type in self.PRIMITIVE_MAP.items(): | ||
| if primitive_kind.value == primitive_name: | ||
| type_str = ts_type | ||
| break | ||
| if not type_str: | ||
| # If not a primitive, treat as a message/enum type | ||
| type_str = self.to_pascal_case(field_type.name) | ||
| elif isinstance(field_type, ListType): |
There was a problem hiding this comment.
generate_type() doesn’t handle qualified type references like Parent.Child. The FDL parser allows dotted names, but to_pascal_case() will leave the dot in place, producing an invalid TypeScript identifier/reference. Consider resolving nested/qualified names (e.g., by flattening or mapping Parent.Child to the generated symbol name) consistently with how nested types are emitted.
| # If not a primitive, treat as a message/enum type | ||
| type_str = self.to_pascal_case(field_type.name) | ||
| elif isinstance(field_type, ListType): | ||
| element_type = self.generate_type(field_type.element_type) |
There was a problem hiding this comment.
For ListType, element_optional isn’t reflected in the generated TypeScript type. Currently list<optional T> / repeated optional T will still emit T[] instead of something like (T | undefined)[], which is a type mismatch.
| element_type = self.generate_type(field_type.element_type) | |
| # Respect optionality of list elements, if available on the IR node. | |
| element_nullable = getattr(field_type, "element_optional", False) | |
| element_type = self.generate_type(field_type.element_type, nullable=element_nullable) | |
| # If the element type is a union (e.g., "T | undefined"), wrap it so | |
| # the array applies to the whole union: (T | undefined)[] | |
| if " | " in element_type: | |
| element_type = f"({element_type})" |
| elif isinstance(field_type, MapType): | ||
| key_type = self.generate_type(field_type.key_type) | ||
| value_type = self.generate_type(field_type.value_type) | ||
| type_str = f"Record<{key_type}, {value_type}>" |
There was a problem hiding this comment.
MapType is emitted as Record<keyType, valueType>, but Record<K, V> requires K extends string | number | symbol. Some valid FDL schemas (e.g., map<int64, ...>) will map to Record<bigint | number, ...> which won’t type-check. Consider normalizing map keys to string (or emitting Map<K, V> / constraining supported key kinds).
| type_str = f"Record<{key_type}, {value_type}>" | |
| # Use Record only for key types allowed by TypeScript's Record<K, V> | |
| if key_type in ("string", "number", "symbol"): | |
| type_str = f"Record<{key_type}, {value_type}>" | |
| else: | |
| # Fallback to Map<K, V> for other key types (e.g., bigint) | |
| type_str = f"Map<{key_type}, {value_type}>" |
| # Register enums | ||
| for enum in self.schema.enums: | ||
| if self.is_imported_type(enum): | ||
| continue | ||
| if self.should_register_by_id(enum): | ||
| type_id = enum.type_id | ||
| lines.append(f" fory.register({enum.name}, {type_id});") | ||
|
|
||
| # Register messages | ||
| for message in self.schema.messages: | ||
| if self.is_imported_type(message): | ||
| continue | ||
| self._generate_message_registration(message, lines) | ||
|
|
||
| # Register unions | ||
| for union in self.schema.unions: | ||
| if self.is_imported_type(union): | ||
| continue | ||
| if self.should_register_by_id(union): | ||
| type_id = union.type_id | ||
| lines.append(f" fory.registerUnion({union.name}, {type_id});") |
There was a problem hiding this comment.
generate_registration() only registers types when should_register_by_id() is true; types without an explicit/generated type_id will be silently skipped, unlike other generators which fall back to namespace/type-name registration. If TS registration is intended to support non-ID schemas, add the named-registration path (or make the omission explicit).
chaokunyang
changed the title
|
The implementation should be something like #3406 |
|
sure @chaokunyang |
Generate TypeScript type definitions and interfaces from FDL IDL for usage with serialization. Features: - Type-safe message interfaces, enums, and discriminated unions - Compatible with both TypeScript and JavaScript - Type ID registration helpers - No runtime dependencies (gRPC-free type definitions) - Proper package name handling and conversion to module names - Support via --typescript_out CLI flag
miantalha45
force-pushed
the
generate-messageTypes-from-IDL
branch
from
c6f379c to
92cd483
Compare
miantalha45
requested review from
PragmaTwice and
chaokunyang
as code owners
|
@chaokunyang I have tried to make implementation according to #3406 . Please have a look on it now and tell me if anything require any change. |
|
And the tests still failed |
|
yes, these tests just are failing, but I am working on fixing these. |
|
|
||
| # Check nested enum | ||
| assert "export namespace person" in output | ||
| assert "export enum phoneType" in output |
There was a problem hiding this comment.
why the generated enum is phoneType instead of PhoneType?
There was a problem hiding this comment.
Have you ever let AI review your code first and address the comments?
There was a problem hiding this comment.
generated enum is phoneType instead of PhoneType because i try to follow the same naming conventions as used in JavaScript file names and also in main runtime.
There was a problem hiding this comment.
which AI? I can't understand what you mean? I have pushed changes almost 3 hours ago, but don't get any comment from AI.
There was a problem hiding this comment.
It just clicked in my mind. structs and enum, class names will follow Pascal Case in JS. let me fix this.
There was a problem hiding this comment.
If you use AI to write code, you should let the AI review your code first
There was a problem hiding this comment.
I don’t let AI perform tasks autonomously. I use it as a tool to assist me with specific tasks.
|
@chaokunyang All tests are passing with compatible mode support. Please have a look on it now and let me know if any changes are required. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (${elemSerializer}) { | ||
| ${foryName}.incReadDepth(); | ||
| ${this.putAccessor(result, `${elemSerializer}.read(${refFlag} === ${RefFlags.RefValueFlag})`, idx)} | ||
| ${foryName}.decReadDepth(); | ||
| } else {${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, `${refFlag} === ${RefFlags.RefValueFlag}`)} | ||
| } |
There was a problem hiding this comment.
readSpecificType builds a generated-code string that contains malformed else blocks (e.g., } else {${...}), which will produce syntactically invalid generated serializer code at runtime. Please restructure the template string so the else { ... } braces and newlines are outside the ${...} interpolation and the block is properly closed.
| } else {${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is another malformed interpolation in the non-TRACKING_REF loop: } else {${this.innerGenerator.readWithDepth(...)} is embedded directly in the generated string, which will lead to invalid generated code / mismatched braces. Please format this as a normal else { ... } block with the ${...} only representing the inner statements.
| } else {${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | |
| } | |
| } | |
| } | |
| } | |
| } else { | |
| ${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | |
| } | |
| } | |
| } |
| const keyTypeId = this.typeInfo.options?.key!.typeId; | ||
| const valueTypeId = this.typeInfo.options?.value!.typeId; | ||
| return keyTypeId === TypeId.UNKNOWN || valueTypeId === TypeId.UNKNOWN | ||
| || !TypeId.isBuiltin(keyTypeId!) || !TypeId.isBuiltin(valueTypeId!); |
There was a problem hiding this comment.
isAny() now treats all non-builtin key/value types (including STRUCT/ENUM/UNION) as "any" and routes them through MapAnySerializer. This changes the wire format/perf characteristics for maps with declared monomorphic user-defined element types (which previously could be serialized without per-entry type info). Consider reverting to the prior isMonomorphic()-based check (or otherwise allowing monomorphic user-defined types) so typed maps remain supported.
| const keyTypeId = this.typeInfo.options?.key!.typeId; | |
| const valueTypeId = this.typeInfo.options?.value!.typeId; | |
| return keyTypeId === TypeId.UNKNOWN || valueTypeId === TypeId.UNKNOWN | |
| || !TypeId.isBuiltin(keyTypeId!) || !TypeId.isBuiltin(valueTypeId!); | |
| const keyTypeInfo = this.typeInfo.options?.key!; | |
| const valueTypeInfo = this.typeInfo.options?.value!; | |
| const keyTypeId = keyTypeInfo.typeId; | |
| const valueTypeId = valueTypeInfo.typeId; | |
| const keyIsMonomorphic = (keyTypeInfo as any)?.isMonomorphic?.() !== false; | |
| const valueIsMonomorphic = (valueTypeInfo as any)?.isMonomorphic?.() !== false; | |
| return keyTypeId === TypeId.UNKNOWN || valueTypeId === TypeId.UNKNOWN | |
| || !keyIsMonomorphic || !valueIsMonomorphic; |
| field_parent_stack = (parent_stack or []) + [type_def] | ||
| props_parts: List[str] = [] | ||
| for field in type_def.fields: | ||
| member = self.safe_member_name(field.name) |
There was a problem hiding this comment.
The generated interface field names use _field_member_name(...) (reserved-word escaping + collision/dedup handling), but registration generation uses safe_member_name(field.name) directly. In the collision/dedup cases this will cause the runtime serializer to look for a different property name than the generated TypeScript type, breaking roundtrip serialization. Please reuse the exact same field-name mapping logic for both interface emission and Type.struct({...props}) registration generation.
| member = self.safe_member_name(field.name) | |
| try: | |
| member = self._field_member_name(field) | |
| except TypeError: | |
| member = self._field_member_name(field, type_def) |
| | Language | Package Usage | | ||
| | ---------- | --------------------------------- | | ||
| | Java | Java package | | ||
| | Python | Module name (dots to underscores) | | ||
| | Go | Package name (last component) | | ||
| | Rust | Module name (dots to underscores) | | ||
| | C++ | Namespace (dots to `::`) | | ||
| | C# | Namespace | | ||
| | JavaScript | Module name (last segment) | |
There was a problem hiding this comment.
Docs say the JavaScript package maps to a "module name (last segment)", but the JavaScript generator derives module/file names from the source file stem when available, otherwise it uses package.replace('.', '_') (not just the last segment). Please update this row to match the generator’s actual behavior to avoid misleading users.
| | Language | Package Usage | | |
| | ---------- | --------------------------------- | | |
| | Java | Java package | | |
| | Python | Module name (dots to underscores) | | |
| | Go | Package name (last component) | | |
| | Rust | Module name (dots to underscores) | | |
| | C++ | Namespace (dots to `::`) | | |
| | C# | Namespace | | |
| | JavaScript | Module name (last segment) | | |
| | Language | Package Usage | | |
| | ---------- | ---------------------------------------------------------- | | |
| | Java | Java package | | |
| | Python | Module name (dots to underscores) | | |
| | Go | Package name (last component) | | |
| | Rust | Module name (dots to underscores) | | |
| | C++ | Namespace (dots to `::`) | | |
| | C# | Namespace | | |
| | JavaScript | Source file stem when available; otherwise dots to underscores | |
docs/compiler/schema-idl.md
Outdated
| | `int16` | `short` | `pyfory.int16` | `int16` | `i16` | `int16_t` | | ||
| | `int32` | `int` | `pyfory.int32` | `int32` | `i32` | `int32_t` | | ||
| | `int64` | `long` | `pyfory.int64` | `int64` | `i64` | `int64_t` | | ||
| | Fory IDL | Java | Python | Go | Rust | C++ | Javascript | |
There was a problem hiding this comment.
Spelling: table header uses "Javascript"; prefer the standard capitalization "JavaScript" (consistent with the rest of the docs).
| | Fory IDL | Java | Python | Go | Rust | C++ | Javascript | | |
| | Fory IDL | Java | Python | Go | Rust | C++ | JavaScript | |
docs/compiler/schema-idl.md
Outdated
| | `uint16` | `int` | `pyfory.uint16` | `uint16` | `u16` | `uint16_t` | | ||
| | `uint32` | `long` | `pyfory.uint32` | `uint32` | `u32` | `uint32_t` | | ||
| | `uint64` | `long` | `pyfory.uint64` | `uint64` | `u64` | `uint64_t` | | ||
| | Fory IDL | Java | Python | Go | Rust | C++ | Javascript | |
There was a problem hiding this comment.
Spelling: table header uses "Javascript"; prefer the standard capitalization "JavaScript".
| | Fory IDL | Java | Python | Go | Rust | C++ | Javascript | | |
| | Fory IDL | Java | Python | Go | Rust | C++ | JavaScript | |
compiler/README.md
Outdated
| ## Features | ||
|
|
||
| - **Multi-language code generation**: Java, Python, Go, Rust, C++, C# | ||
| - **Multi-language code generation**: Java, Python, Go, Rust, C++, C#, Javascript, and Swift |
There was a problem hiding this comment.
Spelling: use the standard capitalization "JavaScript" rather than "Javascript".
| - **Multi-language code generation**: Java, Python, Go, Rust, C++, C#, Javascript, and Swift | |
| - **Multi-language code generation**: Java, Python, Go, Rust, C++, C#, JavaScript, and Swift |
docs/compiler/generated-code.md
Outdated
| PHONE_TYPE_MOBILE = 0, | ||
| PHONE_TYPE_HOME = 1, | ||
| PHONE_TYPE_WORK = 2, |
There was a problem hiding this comment.
The JavaScript enum example shows unstripped values like PHONE_TYPE_MOBILE, but the JavaScript generator strips enum prefixes when possible (e.g., emits MOBILE, HOME, WORK; see test_javascript_enum_value_stripping). Please update this example so it matches the actual generated output.
| PHONE_TYPE_MOBILE = 0, | |
| PHONE_TYPE_HOME = 1, | |
| PHONE_TYPE_WORK = 2, | |
| MOBILE = 0, | |
| HOME = 1, | |
| WORK = 2, |
|
|
||
| fileRoundTrip("DATA_FILE_COLLECTION", 210, registerCollectionTypes, { compatible }); | ||
|
|
||
| // DATA_FILE_COLLECTION_UNION: NumericCollectionUnion (IS a union) |
There was a problem hiding this comment.
Please also add union support in this pr too instead of skipping it. Union is a generated schema idl feature. Normal javascript code can't express union concept.
There was a problem hiding this comment.
Sure, let me check how we can add union support in JS IDL compiler. Union serialization is already supported in JS runtime but not in IDL generator.
| computeStructHash() { | ||
| const fields = TypeMeta.groupFieldsByType(this.fields); | ||
| const fingerprint = this.computeStructFingerprint(fields); | ||
| // const fields = TypeMeta.groupFieldsByType(this.fields); |
There was a problem hiding this comment.
why this is commented out?
You comment out groupFieldsByType and feeds raw this.fields directly into computeStructFingerprint. Java and other runtimes group fields by type before hashing. This means the JS struct hash will differ from Java's for any struct whose field ordering changes after grouping, causing schema-consistent mode to reject valid payloads with a hash mismatch error.
There was a problem hiding this comment.
computeStructFingerprint sorts fields alphabetically by field identifier regardless of input order, so even if the grouping were different, the resulting hash would be the same.
but your point is also valid, i will uncomment it and will group fields by type.
| const typeId = typeMeta.getTypeId(); | ||
| const userTypeId = typeMeta.getUserTypeId(); | ||
| if (!TypeId.structType(typeId)) { | ||
| const existing = this.fory.typeResolver.getSerializerById(typeId, userTypeId); |
There was a problem hiding this comment.
Why add this? only struct serializer has type meta
There was a problem hiding this comment.
This was added to handle non-struct types (like unions) that go through the TypeMeta path during compatible-mode deserialization. In struct.ts, the default case calls genSerializerByTypeMetaRuntime for any typeId in compatible mode including unions. Without this fallback, encountering a union TypeMeta would throw "only support reconstructor struct type" even though a serializer for it is already registered.
There was a problem hiding this comment.
the union should not create type meta. have you ever see how other languages implement generated union?
| { | ||
| const bytes = this.scope.declare("typeInfoBytes", `new Uint8Array([${TypeMeta.fromTypeInfo(this.typeInfo).toBytes().join(",")}])`); | ||
| typeMeta = this.builder.typeMetaResolver.writeTypeMeta(this.builder.getTypeInfo(), this.builder.writer.ownName(), bytes); | ||
| typeMeta = this.builder.typeMetaResolver.writeTypeMeta(this.ownTypeInfoExpr, this.builder.writer.ownName(), bytes); |
There was a problem hiding this comment.
The constructor already computes this.ownTypeInfoExpr. Extract a single serializerExpr string field in the constructor
and reuse it everywhere.
There was a problem hiding this comment.
sure, let me do this.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
javascript/packages/core/lib/meta/TypeMeta.ts:1
- This changes the input to
computeStructFingerprint(previously grouped/categorized fields, now rawthis.fields) and leaves a commented-out line behind. If the struct fingerprint/hash participates in schema evolution or cross-language compatibility, this can change the computed hash for existing schemas and break compatibility. Recommendation (mandatory): either restore the previous grouping behavior, or updatecomputeStructFingerprint(and any cross-language spec/tests) so that the new hashing scheme is explicitly defined and consistent across languages; also remove the commented-out dead code.
javascript/packages/core/lib/gen/union.ts:1 getFixedSize()returns a constant (12) for union serialization, but union payloads include varints, ref flags, and nested type-info/value writes that are not fixed-size. IfgetFixedSize()is used to drive buffer reservation or fast paths, a fixed constant is misleading and can cause under/over-reservation. Prefer returning a sentinel for variable-sized serializers (if the framework supports it) or computing a conservative upper bound based on the encoding actually emitted.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for field in type_def.fields: | ||
| member = self.safe_member_name(field.name) | ||
| expr = self._field_type_expr(field.field_type, field_parent_stack) | ||
| if field.tag_id is not None and field.tag_id > 0: | ||
| expr += f".setId({field.tag_id})" | ||
| if field.optional: | ||
| expr += ".setNullable(true)" | ||
| if field.ref: | ||
| expr += ".setTrackingRef(true)" | ||
| props_parts.append(f"{member}: {expr}") |
There was a problem hiding this comment.
The registration helper uses safe_member_name(field.name) directly, but the emitted TypeScript interface fields use _field_member_name(...) (which deduplicates and avoids collisions with reserved words and nested type names). This can make runtime serialization keys diverge from the generated interface keys (e.g., when a field name collides with a nested type name and gets renamed to ...Value in the interface). Fix by reusing the same field-name resolution logic in registration (e.g., compute used_field_names and call _field_member_name(field, message, used_field_names)), or by maintaining a single canonical name mapping used by both interface emission and registration.
| def get_module_name(self) -> str: | ||
| """Get the TypeScript module name from source file or package.""" | ||
| if self.schema.source_file and not self.schema.source_file.startswith("<"): | ||
| return Path(self.schema.source_file).stem | ||
| if self.package: | ||
| return self.package.replace(".", "_") | ||
| return "generated" |
There was a problem hiding this comment.
Docs/PR description state JavaScript uses the last segment of the package name for module naming, but the generator uses package.replace('.', '_') when source_file is not available. Please align behavior and documentation: either change get_module_name() to use the last segment (e.g., self.package.split('.')[-1]) or update the docs/PR description to match the underscore-joined module naming.
| const serializerExpr = TypeId.isNamedType(this.typeInfo.typeId) | ||
| ? `fory.typeResolver.getSerializerByName("${CodecBuilder.replaceBackslashAndQuote(this.typeInfo.named!)}")` | ||
| : `fory.typeResolver.getSerializerById(${this.typeInfo.typeId}, ${this.typeInfo.userTypeId})`; |
There was a problem hiding this comment.
The embed paths inline fory.typeResolver.getSerializerBy*() multiple times per read/write, which can repeat resolver lookups within hot loops (e.g., collections/struct fields). Previously this was typically cached via scope.declare(...). Consider declaring a local const ser = ... once per generated read/write method (or once per embed handler) and reuse it for .readTypeInfo()/.read()/.write() to avoid repeated resolver lookups.
| ${serializerExpr}.readTypeInfo(); | ||
| fory.incReadDepth(); | ||
| let ${result} = ${serializerExpr}.read(${refState}); |
There was a problem hiding this comment.
The embed paths inline fory.typeResolver.getSerializerBy*() multiple times per read/write, which can repeat resolver lookups within hot loops (e.g., collections/struct fields). Previously this was typically cached via scope.declare(...). Consider declaring a local const ser = ... once per generated read/write method (or once per embed handler) and reuse it for .readTypeInfo()/.read()/.write() to avoid repeated resolver lookups.
| break; | ||
| case "javascript": | ||
| workDir = idlRoot.resolve("javascript"); | ||
| command = Arrays.asList("npx", "ts-node", "roundtrip.ts"); |
There was a problem hiding this comment.
The JS peer command runs npx ts-node roundtrip.ts but doesn’t ensure integration_tests/idl_tests/javascript dependencies are installed. This can make the Java-driven roundtrip tests flaky/fail in clean environments (missing ts-node, typescript, and the file-based @apache-fory/core dependency). Consider switching this peer command to run through npm (e.g., npm ci/npm install once, then npm run roundtrip), or update the test harness to install dependencies before invoking the peer.
| command = Arrays.asList("npx", "ts-node", "roundtrip.ts"); | |
| command = | |
| Arrays.asList("sh", "-c", "npm install && npx ts-node roundtrip.ts"); |
| python "${SCRIPT_DIR}/generate_idl.py" --lang javascript | ||
|
|
||
| cd "${SCRIPT_DIR}/javascript" | ||
| npm install |
There was a problem hiding this comment.
For CI determinism and speed, prefer npm ci (when a lockfile is present) over npm install. If you intentionally do not commit a lockfile for these integration tests, consider documenting that choice and/or pinning tool versions another way to reduce dependency drift.
| npm install | |
| if [ -f package-lock.json ]; then | |
| npm ci | |
| else | |
| npm install | |
| fi |
| } else { | ||
| ${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | ||
| } | ||
| } |
There was a problem hiding this comment.
those } looks very strange
| } | ||
| } | ||
|
|
||
| } else { | ||
| for (let ${idx} = 0; ${idx} < ${len}; ${idx}++) { | ||
| ${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | ||
| if (${elemSerializer}) { | ||
| ${foryName}.incReadDepth(); | ||
| ${this.putAccessor(result, `${elemSerializer}.read(false)`, idx)} | ||
| ${foryName}.decReadDepth(); | ||
| } else {${this.innerGenerator.readWithDepth((x: any) => `${this.putAccessor(result, x, idx)}`, "false")} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The read path has been heavily modified with new if (${elemSerializer}) branches, but the brace nesting looks broken — there
are unmatched } on lines that should be inside else blocks, and the indentation is inconsistent with the surrounding code.
| | Swift | `Person.PhoneNumber` | | ||
| | Language | Nested type form | | ||
| | ---------- | ------------------------------ | | ||
| | Java | `Person.PhoneNumber` | |
There was a problem hiding this comment.
those should be Person.PhoneNumber style
Summary
Implements TypeScript code generation for Fory IDL within the fory-compiler, converting FDL (Fory Definition Language) schema files into pure TypeScript type definitions. Zero runtime dependencies, with comprehensive test coverage (12/12 tests passing), supporting messages, enums, unions, and all primitive types.
Changes
Core Implementation
compiler/fory_compiler/generators/typescript.py - TypeScript code generator extending BaseGenerator (365 lines)
compiler/fory_compiler/generators/init.py - Registration of TypeScriptGenerator in the compiler ecosystem
compiler/fory_compiler/cli.py - Added --typescript_out CLI argument for TypeScript code generation
compiler/fory_compiler/tests/test_typescript_codegen.py - 12 golden codegen tests covering:
Features
AI Assistance Checklist
AI Usage Disclosure
Fixes #3280