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

autoLimit in context of graphql calls #451

Closed pociej closed 8 months ago

pociej commented 3 years ago

As we know one can set autoLimit to avoid hiting rate limit. Problem is that ( from documentation )

When using an object, the calls property and the interval property specify the refill rate and the bucketSize property the bucket size

While in graphql api context limits are not calculated based on calls but points as described here https://shopify.dev/concepts/about-apis/rate-limits.

lpinca commented 3 years ago

The autoLimit option is ignored when using the GraphQL API. This is documented here https://github.com/MONEI/Shopify-api-node#shopifycalllimits

pociej commented 3 years ago

I missed that indeed. Anyways, issue is still the same. Queuing calls to make sure one dont hit limit should should be on api wrapper level. And in case of REST API it is. What i reason you didnt implement this on the graphql level?

lpinca commented 3 years ago

Because the rate limiting method is different. Rate limiting for the REST API is based on the token bucket algorithm and we use https://github.com/lpinca/stopcock. This does not work for the GraphQL API, see https://github.com/MONEI/Shopify-api-node/pull/282#issuecomment-501310673.

pociej commented 3 years ago

Of course i know it. If you would reread my first comment in this thread you could see I linked shopify docs that describes it. However fact that rate limit is different doesn't mean package should not support graphql case. autoLimit could for example have one more level and be of the form autoLimit : { rest : {}, graphQl : {} }. With current implementation one can rely on the package when it comes to REST rate limits but for graphql not and monkey patching is needed. It doesn't sound good. Right?

lpinca commented 3 years ago

However fact that rate limit is different doesn't mean package should not support graphql case.

No one said there should be no support for it. It is not implemented.

pociej commented 3 years ago

Ah ok, so we misunderstood each other ( or at least i misunderstood you :) ). I was sure you are trying to convince me that it is intentional and should be reported as issue. I will PR in free moment.

lpinca commented 8 months ago

I'm closing this due to inactivity.