Skip to content

Conversation

@joe-p
Copy link

@joe-p joe-p commented Jul 22, 2025

Currently Optional types in records are required fields in the generated typescript. This PR replaces the undefined union with a ? to indicate that the field is optional. This results in more idiomatic TypeScript.

The diff of the generated TypeScript can be seen in this commit: algorandfoundation/algokit-core@30d9488

@zzorba
Copy link
Collaborator

zzorba commented Jul 23, 2025

I think previously we were using the concept of a uniffi 'default' to mark something as required/optional, but it seems like that feature is not super widely used in Uniffi.

@joe-p does this better align with what you've seen in other uniffi bindings?

I'm supportive of this change, though perhaps it should be configurable? Just thinking out loud, don't feel strongly either way.

@joe-p
Copy link
Author

joe-p commented Jul 23, 2025

@joe-p does this better align with what you've seen in other uniffi bindings?

My main experience so far is with the Python bindings, which do use typing.Optional meaning they can be omitted, which aligns with that this PR does. For example:

class Transaction:
    transaction_type: "TransactionType"
    """
    The type of transaction
    """

    sender: "str"
    """
    The sender of the transaction
    """

    fee: "typing.Optional[int]"
    """
    Optional transaction fee in microALGO.

    If not set, the fee will be interpreted as 0 by the network.
    """

    first_valid: "int"
    last_valid: "int"
    genesis_hash: "typing.Optional[ByteBuf]"
    genesis_id: "typing.Optional[str]"
    note: "typing.Optional[ByteBuf]"
    rekey_to: "typing.Optional[str]"
    lease: "typing.Optional[ByteBuf]"
    group: "typing.Optional[ByteBuf]"
    payment: "typing.Optional[PaymentTransactionFields]"
    asset_transfer: "typing.Optional[AssetTransferTransactionFields]"
    asset_config: "typing.Optional[AssetConfigTransactionFields]"
    application_call: "typing.Optional[ApplicationCallTransactionFields]"
    key_registration: "typing.Optional[KeyRegistrationTransactionFields]"
    asset_freeze: "typing.Optional[AssetFreezeTransactionFields]"                  

though perhaps it should be configurable

It could be, but this PR isn't a breaking change and semantics are preserved so I don't think it needs to be. Fwiw I've never seen an undefined union in a type/interface definition in the wild, so I think ? is more idiomatic.

Also I see CI is failing. Presumably I need to regenerate some generated bindings somewhere, but will further investigate today or tomorrow.

@jhugman
Copy link
Owner

jhugman commented Sep 2, 2025

Also I see CI is failing. Presumably I need to regenerate some generated bindings somewhere, but will further investigate today or tomorrow.

I think the tests are failing because undefined is being used in the tests themselves instead of omitting the property or argument.

On the whole, I don't have any feeling one way or another about this but have these cautionary tales / silly questions to tell:

  • we have more and more users, so surprising non-optional breakages should be kept close zero. I think I'd agree with @zzorba in asking for this be triggered on a property in uniffi.toml.
  • how does this interact with default values?
  • when adding default values to python, we found that python supports default arguments only in the final arguments in a list. I'm not sure how the parsing works in Typescript, or even if we should mirror Python's behavior if Typescript allows it: in a general case, I don't think I'd like to be able write Rust which could be called from Typescript, but not from Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants