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

Add ability to use a custom JSON parser and serializer #502

Closed lpinca closed 3 years ago

lpinca commented 3 years ago

Fixes: https://github.com/MONEI/Shopify-api-node/issues/500

lpinca commented 3 years ago

cc: @airhorns

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 4ed4d10ca92f44a15c930c189931159605afd975 on fix/issue-500 into c8b5cdb5900fe687fc9a4488b230a3636ea32450 on master.

airhorns commented 3 years ago

Nice! Do you think it makes sense to set json-bigint as the defaults since we know that Shopify is gonna return big integers? Make it work by default for users so they don't have to care? Without the defaults, I feel like there will be a stream of sad people who get hit with this very confusing error in production and have to go spelunking to find the issue and the remedy and then set the default to json-bigint which seems like a bit of wasted effort.

It would change the minimum node version from 10.0 to 10.4 which doesn't seem too bad to me, but I think strictly speaking it'd also be a breaking change since downstream callers might not expect bigints to come back.

lpinca commented 3 years ago

but I think strictly speaking it'd also be a breaking change since downstream callers might not expect bigints to come back.

Yes, this is why I prefer to use JSON.parse() and JSON.stringify() as defaults and let users use a custom parser/serializer if they need to. It would also be easier for users who wants to experiment with https://github.com/tc39/proposal-json-parse-with-source, without relying on json-bigint or another dependency.

lpinca commented 3 years ago

I've tried to make json-bigint parse() and stringify() the defaults but I hit this bug https://github.com/sidorares/json-bigint/issues/49 for https://github.com/MONEI/Shopify-api-node/blob/3.6.14/test/fixtures/draft-order/res/list.json#L59.

json-bigint is currently a no go. Do you know any other safe alternative? We can also try to fix that issue but I'm not sure if json-bigint is actively maintained.

airhorns commented 3 years ago

That is a big issue with json-bigint, yeesh. No go indeed. I don't know of any alternatives which again seems nuts to me that tonnes of people aren't hitting this problem ... but alas. I will see if I can upstream a fix to json-bigint or dig something else up!

lpinca commented 3 years ago

There is also another issue with json-bigint. If a number has more than 15 digits (including the dot for floats) it will parse it using bignumber.js / BigInt even if it is not needed. For example, there is no need to use bignumber.js for -75.69314750000001 and I expect it to be parsed as a plain JS number.

lpinca commented 3 years ago

I think I'll merge this as is and let users deal with these issues themselves.

weeksie commented 1 year ago

This is horrifying. The answer to client developers getting silently wrong values is to shrug and say I dunno, we couldn't figure it out, you do it. (But, like, give very little indication that this will be an issue in the docs)

I was just going to move over from doing raw fetch/rest calls but I guess I won't now? The Shopify ecosystem has been a non stop parade of surprises, that's for sure