search

graphprotocol / graph-client

The Graph library for building GraphQL-based dapps in a decentralized way.
MIT License
174 stars 44 forks source link

Native support for data type of `BigInt` #90

Open dotansimha opened 2 years ago

dabbott commented 2 years ago

Note that true BigInt support may be a bit tricky for React Native, since React Native's Hermes JS engine doesn't support the BigInt type yet. It is in the works though https://github.com/facebook/hermes/issues/510!

matthewlilley commented 2 years ago

Yeah, I'm a bit hessitent to enforce BigInt just yet. There's good but not enough support for all users, yet. We use JSBI to represent BigInt in our dApps, which I think would be a great middle ground.

https://github.com/GoogleChromeLabs/jsbi

ardatan commented 2 years ago

Available in the latest release!

matthewlilley commented 2 years ago

@ardatan Is there some way to disable this? We don't use native bigint for a few reasons, one being that we aren't able to serialize it, and do a lot of SSR. We use JSBI in our SDKs also to provide maximum browser coverage. This is a bit of a pain for us atm and preventing updating.

matthewlilley commented 2 years ago

Worked around for now, but would be nice if we were able to transform BigInt & BigDecimal ourselves, depending on needs.

ardatan commented 2 years ago

@matthewlilley Graph Client uses json-bigint-patch to handle BigInt parsing and serialization. BigInts are primitive of JavaScript today and supported by the major browsers. Could you explain more about the issue you have? Maybe we can help you.

matthewlilley commented 2 years ago

@ardatan I'm unable to build with type safety enabled now.

Screenshot_804

If I cast to BigInt, like so

Screenshot_805

Error: Do not know how to serialize a BigInt

matthewlilley commented 2 years ago

I think it's bad to assume and even worse enforce that everyone would like native bigint support based on the schema, since the majority of subgraphs written use BigInt for convininance within mappings, not because they want to be parsed as that on the client.

And in a case like this it makes even less sense, because it's a timestamp which will never be BigInt, it's a unsigned 32-bit integer...

Just for context, this totally breaks Next.js, it makes caching impossible because it expects serializability via JSON.stringify.... this should be opt-in/out, I expect many other frameworks and libraries will suffer from issues with this too.

I'm forced to // @ts-ignore in many places or unable to compile.

Maybe type BigIntIsh = bigint | number | string makes more sense?

It's parsing ERC20 decimals as BigInt too, this is a unsigned 8 bit integer, it makes no sense to ever be represented as a BigInt.

This update was also a breaking change, but wasn't versioned this way.

azf20 commented 2 years ago

Agree with @matthewlilley, often users will use BigInt simply for convenience / as a default, and users don't expect the native type

matthewlilley commented 2 years ago

Agree with @matthewlilley, often users will use BigInt simply for convenience / as a default, and users don't expect the native type

To contiue this conversation, we plan to eventually migrate to native bigint, but given the speed at which we can iterate subgraphs this would need to happen over a long time period, so support for incremental adoption of native bigint , or other transforms is critical.