-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JS v3: Native bigint support for select fields #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm wondering why should support all of these options in property files, what's the use case for clients needing the ability to override?
# otherwise operated on with a bigint, it should be a bigint for ease of use | ||
# * Other quantities outside of the above and which will never exceed the maximum | ||
# safe javascript integer should be number | ||
type_override_Account_amount=bigint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question, why should we leave this as configurable at all? While we are putting sane defaults here, what are the cases where we should support the client overriding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I should explain what's happening in this file. It's really just a list of key-value definitions.
In the template code, specifically here:
generator/typescript_templates/model.vm
Lines 7 to 10 in 4b0120a
#set( $d = "$" ) | |
#set( $e = "!" ) | |
#set( $type_override_variable = "${d}${e}propFile.type_override_${className}_#paramName(${param})" ) | |
#set( $type_override = "#evaluate($type_override_variable)" ) |
we look at propFile.type_override_X_Y
, where X
is the generated class name and Y
is the field in that class. If there's a value associated with that key from the prop file, we will use the type override that it declares.
So it's less about making each field configurable, and more about configuring the list of fields and their overrides.
I chose to put the overrides in this config file for a few reasons:
- Different fields needed to be overriden for algod and indexer
- It's much easier to encode this information in a file like this instead of placing a massive if/elseif/.../else statement in the template code
Currently JS types every numeric field as
number | bigint
. This isn't very useful, since you can't operate directly against this union type.This PR allows the generated types to use either
number
orbigint
, depending mostly on thex-algorand-format: uint64
tag on fields. Previously this tag was only used to decided whether the Java SDK should useLong
orBigInteger
for a field.However, there are two reasons why that approach can only go so far:
bigint
in cases where Java'sLong
would be sufficient.x-algorand-format: uint64
tag is lacking/inconsistent in some places.To address these issues, I added the ability to manually override types by adding to one of the property files.
For consistency, I made the following quantities always have type
bigint
, even if they weren't declared withx-algorand-format: uint64
in the REST spec:upgradeDelay
)These changes are present in this JS SDK PR: algorand/js-algorand-sdk#852