search

kirillplatonov / shopify_graphql

Less painful way to work with Shopify Graphql API in Ruby.
MIT License
59 stars 9 forks source link

Use Struct over OpenStruct #24

Closed remy727 closed 11 months ago

remy727 commented 1 year ago

Motivation / Background

Struct is faster than OpenStruct generally.

kirillplatonov commented 1 year ago

Good idea 👍

resistorsoftware commented 1 year ago

What happened with this? Is it still the same argument in that Struct is fast and OpenStruct is slow? Is it really just that choice? It seems like we'd never alter the results, just consume them, so Struct does make sense and is faster?

resistorsoftware commented 1 year ago

Just a quick question on this, that has come into play a few times. Sometimes you have to return Struct attributes that have no value. As in 

source: edge.node.featuredImage&.source

So when consuming this in a Ruby sense, we'd end up with data.source perhaps being nil. Which is fine. But checking on nil is a thing, and then there is straight-up returning an empty string, which can be useful. Or a default value. So for example, say you wanted to return some default image here in the struct.

How do you look at that? I like the code! I am just finding that when I use it a lot, I run into these issues and they grow out into support code that becomes almost indispensable. Any thoughts?

remy727 commented 1 year ago

I think you can use || operator.

source: edge.node.featuredImage&.source || default_image
remy727 commented 1 year ago

@kirillplatonov, this is ready for review. I also tested and all passed.

kirillplatonov commented 11 months ago

@remy727 I dived a bit deeper into this idea. The majority of OpenStruct use is happening during Graphql response parsing:

JSON.parse(response.body.to_json, object_class: OpenStruct)

https://github.com/kirillplatonov/shopify_graphql/blob/main/lib/shopify_graphql/client.rb#L20

Unfortunately, switching parser object class to Struct is not possible as it will lead to TypeError: allocator undefined for Struct error. And I don't know how to implement auto-conversion of responses without OpenStruct.

With this in mind, changing OpenStruct to Struct in response wrappers doesn't makes much sense. It will decrease code readability without significant performance impact.

remy727 commented 11 months ago

@kirillplatonov, that makes sense. Thanks for diving into it.