Closed BoldBrandonM closed 8 months ago
BoldBrandonM
commented
1 year ago In the short term, I plan to use the WithHttpClient client option to supply a custom context-aware transport layer and inject the notification ability. It's not a perfect solution but will allow me to wait until we release a breaking change to the interfaces.
oliver006
commented
10 months ago @BoldBrandonM - #5 is seeing some movement and we're adding context to all Client actions. This will be a breaking change, what other breaking changes should go out with that one? Anything for this issue here? I like the idea of adding response headers to the context via defined constants. Would that sufficiently address your needs?
BoldBrandonM
commented
10 months ago The changes in #5 do seem like they would work well bundled with what you mention here, and I think that would sufficiently address our needs for this issue.
I've also been tracking these over the years and feel that they would be a beneficial inclusion for a major version upgrade (since we can't really address them any other time):
oliver006
commented
10 months ago re: #4 - what does the solution to this look like? Turn every field that has "omitempty" into a pointer?
The other issues are addressed in #258 (have a look if you have a moment and let me know if the changes look sane to you)
BoldBrandonM
commented
10 months ago @oliver006 I would say that you're about right. Either pointer types or non-primitives (struct representations of nullability). It's certainly a massive undertaking which will require lots of changes for clients of this library, so I understand concerns with adopting it.
A non-breaking version could only address the primitives issue but it would need to introduce transformations to / from pointers to the primitive types without exposing the pointer on the external interface. That would simply allow us to use zero values, but not null as-such.
Another alternative would be to stop omitting empty, also large and sweeping but puts a lot of pressure on the caller to adopt the change or else face very nasty side-effects (likely many are sending partial updates today, which would need different support).
As for the changes in #258 I'll take a look over the next couple days, things are a little busier over here lately.
BoldBrandonM
commented
10 months ago I'll also bring the team's attention to this and see if we can get a few more eyes.
oliver006
commented
10 months ago Thanks Brandon.
I'm a bit on the fence re: #4 - that seems like a massive change and one that I personally don't have the time for right now. I'd be happy to shepherd the remaining changes in #258 plus the context changes in #257 over the finish line but I can't commit to another large PR right now for the omitempty changes.
Hi all, I am looking at an issue where some portion of our codebase is making API calls to deprecated API versions. Since Shopify themselves only inform of the affected applications and the date of the call in their admin UI, and only notifies the caller in http response headers, the current implementation doesn't give us sufficient granularity to action the required change.
Since this library does not expose the http response object, nor the response headers, we're currently in a bit of a pickle.
This relates to https://github.com/bold-commerce/go-shopify/issues/5 in particular, whenever the change to accept request context in the interfaces is actioned, response headers could be returned on a derived context or on some well-defined key for the Shopify request in the input context. Until then we may need a short term opt-in solution which does not break the interfaces for all consumers.