search

MONEI / Shopify-api-node

Node Shopify connector sponsored by MONEI
https://monei.com/shopify-payment-gateway/
MIT License
946 stars 278 forks source link

BigInt ID handling #500

Closed airhorns closed 3 years ago

airhorns commented 3 years ago

Shopify's REST APIs produce ID numbers in the JSON responses that require more than 53 bits of precision to represent, which when parsed with the plain old JSON.parse built in lose precision, resulting in issues like these: https://community.shopify.com/c/Shopify-APIs-SDKs/Int-too-big-to-fit-in-my-API/td-p/1070054

As best I can tell, this library uses JSON.stringify and JSON.parse (via got) to send and receive JSON, which means that the id numbers sent and returned from various calls will be incorrect! I hope I am missing something, but otherwise, I think it would be good for this library to do something to help users properly parse (and send) bigints to/from Shopify.

In the past, solutions like json-bigint used userland implementations of arbitrary precision numbers like big.js, but now, newer ES versions have the bigint type built in. json-bigint allows that type to be used natively for parsing and stringifying with the useNativeBigint option, so would it make sense to switch to it for all that and ask users to use native bigints to represent big IDs?

lpinca commented 3 years ago

To be honest I think this is a Shopify issue. Other APIs use strings for 64 bit IDs because most JSON parsers use the same precision as the IEEE 754 double for numeric values.

Anyway this should eventually be opened in the got issue tracker as JSON parsing and serialization is done by got. We call JSON.stringify() explicitly but only for GraphQL.

airhorns commented 3 years ago

To be honest I think this is a Shopify issue.

Yeah, I don't disagree, but I don't think they are gonna change it any time soon. I used to work there and when they switched to 64 bit ints internally in like 2014 they specifically decided not to do strings as output because it would cause such a huge amount of downstream pain for everyone, and a lot of not-JS languages (like Ruby) parse big numbers in JSON just fine. I agree it is super annoying though.

Anyway this should eventually be opened in the got issue tracker as JSON parsing and serialization is done by got.

Yeah, I think if you peel back the onion enough, it really should be the default behaviour for JSON.parse and JSON.stringify if you ask me, so it works for got and axios and node-fetch and and and and. But, TC-39 hasn't yet decided on it I don't think. In the meantime, are you open to fixes that make it better for this API specifically which we know emits big ints?

lpinca commented 3 years ago

I'm not against it but we would need to JSON stringify and parse ourselves, adding the correct headers ourselves, and handling parse errors ourselves which is one of the main reasons why got is used in the first place. How common are IDs greater than Number.MAX_SAFE_INTEGER?

airhorns commented 3 years ago

I think you might be able to use parseJson and stringifyJson? https://github.com/sindresorhus/got/blob/main/documentation/2-options.md#parsejson

And I am not sure how common it is, I am kind of surprised that this is the first issue about it because I've been burned by this problem on a number of Shopify projects. Personally I think its a bad enough bug for the people that do it it that it's worth fixing, but not really up to me! The alternative would be monkeypatching the JSON global which feels ... pretty gross.

lpinca commented 3 years ago

Sounds good. If got supports a custom JSON.parse() and JSON.stringify() I'm fine with using json-bigint.

However we would need to bump the minimum Node.js supported version to 10.4.0.

lpinca commented 3 years ago

FYI, we were doing manual parsing and serialization for GraphQL in v2, see https://github.com/MONEI/Shopify-api-node/blob/2.25.1/index.js#L222-L230, so that is also an option but I would prefer to avoid that.