fix(client): parse Presto JSON response with custom reviver and BigInts

[alonsovb]

Fixes #24. For now, this only assumes the JSON.parse includes the context.source in the reviver callback. This is currently a stage 3 proposal but has very good browser support.

Changes

Uses a custom JSON.parse reviver for numbers so that everything uses the BigInt primitive instead of the regular Number which loses precision on bigger values.

Notes

None

Screenshots

None

[yhwang]

Thanks for the PR and the changes look good to me.
Just left a minor question.

[alonsovb]

I refactored the code to do the compat check in the same function by checking if the context (and context.source) values are non falsy.

The only weird thing that happens is if you try to stringify the query response since now it contains a BigInt which is not serializable by the default JSON.stringify function. You need to write a custom replacer like this:

const results = await client.query(`SELECT 1234567890123456623`)
return {
  result: JSON.stringify(results.data, (key, value) => {
    if (typeof value !== 'bigint') return value

    return value.toString()
  }),
}

Otherwise you may end up with an error like this:

ERROR [ExceptionsHandler] Do not know how to serialize a BigInt
TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)

We could either:

1. Leave it like this (up to users on how to serialize BigInts, a bit more work)
2. Automatically parse into string instead of BigInts (lose ability to do some number operations on them unless customly parsed into BigInts)
3. Some kind of param to decide whether to use 1 or 2 per client or per query.

Any advice @yhwang ?

[jorgeramirezamora]

Some comments below

[yhwang]

@alonsovb per your question. My 2 cents:
We should provide APIs like the Response object. To get the output in text form, use response.text() API, to get the output in JSON form, use response.json() API. In our case, we should provide similar APIs, one for raw data or text, the other for the PrestoQuery which has been parsed and converted to BigInt if needed. This would prevent users from getting the parsed data and then stringifying it.

However, this requires API changes. We should document the limitation like what you did now and create an issue to track this and discuss the proper solution there. Your option 2 is also doable and no API change.

[alonsovb]

Okay, just finished adding some initial unit tests. Let me know if anything else may be needed cc @yhwang @jorgeramirezamora

Per @yhwang , I created this issue to discuss more about a potential API change for the Client's response object:
#26

Thanks everyone!

[jorgeramirezamora]

👏🏼

[jorgeramirezamora]

❤️

[jorgeramirezamora]

🤯