Use `decimal.js` to represent decimals

[colinhacks]

We currently don't implement a codec for decimals. I vote we use the Decimal.js to represent these values until a proper Decimal type lands in JS. It's a de facto standard for representing decimals in JavaScript. No other serious alternatives exist. Long term we can implement a system where users can plug in their own custom codecs, but in the vast majority of cases users who are using decimal values will be relying on Decimal.js, so we should make this the happy path. As of NPM v7, we can now include it as an optional peer dependency which was introduced for scenarios exactly like this: https://stackoverflow.com/questions/62047806/how-do-i-set-a-peer-dependency-optional

[1st1]

I'm -1 on depending on decimal.js directly. It can be an optional dependancy and we can have some API to DI it, a la client = client.withDecimalJS().

[jaclarke]

Maybe the api should allow you to pass in a constructor for your decimal type, since decimal.js has a configurable constructor api (http://mikemcl.github.io/decimal.js/#Dclone), and this would also allow you to use any other decimal library if you wanted.

~Though I'm not entirely sure how this would interact with typing in the querybuilder. I guess client would gain a generic type param, that we would pass through to computeTsType in the run method?~

Edit: I think having the api be on client also doesn't play well with the .edgeql workflow proposal (#314), the static interfaces we generate for the querybuilder or input types for e.insert etc. So alternative idea: We have an optional config file from which the user exports the the decimal constructor. Then the querybuilder generator (and .edgeql workflow generator) can check this config file and statically generate the correct types.

[colinhacks]

As I mentioned on Slack: I'm also happy just returning decimal values as strings (which can then be passed into decimal.js or any equivalent library, should the user need to perform operations on them. Just using strings is better than not implementing a codec at all imo.

Though I'm not entirely sure how this would interact with typing in the querybuilder. I guess client would gain a generic type param, that we would pass through to computeTsType in the run method?

Yep this is how it would work. I think it would work actually, even with the *.edgeql workflow. Basically we'd use some unique type as a stand-in for a Decimal type (perhaps a string literal or something like {__type__: "Decimal"}). Then we'd write a recursive type alias to "post-process" the return type of .run() using the generic "decimal type" parameter of the client you pass into .run.

Here's a proposed API for this. I'm not in love with decimalHandler as the name of the setting.

const client = createClient()
  .withOptions({ decimalHandler: (arg: string)=>new Decimal(arg) })

So alternative idea: We have an optional config file from which the user exports the the decimal constructor

This seems tricky. The config file approach requires the generated query builder files to know exactly where the config file is. We'd need to know the location of the config file at generation time, then read the value of --output-dir (if provided) during generation, and "hard code" an import into the query builder. We'd also need to prevent this import from happening if the config file does exists to avoid creating an import to a non-existent file.

[goatyeahh]

Just using strings is better than not implementing a codec at all imo.

I agree currently the only way to make queries with decimals work is to do explicit casting for every decimal pointer. This makes the code harder to read and to maintain.

Example of code with a none nested query with only one decimal (I will let you imagine nested multiple decimal pointers):

const invoices = e.select(e.Invoice, (item) => ({
  cast_amount: e.cast(e.str, item.amount),
}))
invoices.map((invoice) => {
    invoice.amount = invoice.cast_amount;
    delete invoice.cast_amount;
  })

Maybe there is a cleaner way to tackle this?

Honestly, any option (return strings, DI or peerDependency on decimal.js) is better than the current implementation even if you change your mind later.

[lu-zen]

any update on this? got this error: InternalClientError: no JS codec for std::decimal

[raddevon]

Our approach to this will be to allow for decoding numeric types as strings so that users can send the string values to a codec of their choice. Look for it to ship within the next week or two.