Shopify.Pagination module #92

balexand · 2019-12-18T23:27:06Z

Fixes #83. See the doc comments in the Shopify.Pagination module for details and examples.

balexand · 2020-01-01T20:29:36Z

It's possible that the :middleware option can be removed in order to simplify things. A custom adapter could be used to throttle requests instead. Please let me know if you have any preferences. Happy new year!

larskluge · 2020-03-06T05:52:19Z

Thanks @balexand for your contribution. @nsweeting any chance you have some feedback on this for us all? Shopify will remove for page based navigation in less than a month now: April 1st. Thank you both!

balexand · 2020-03-06T22:11:06Z

Let me know if there's anything that I can do to help. I'm currently using the code from this PR in production and it works great.

ProtoJazz · 2020-05-28T17:57:28Z

@balexand since this doesn't seem to be getting merged in I've been using your fork. I wonder if it might be worthwhile to publish a new hex package. There's still plenty of stuff that could be added, I noticed some missing fields on some resources, and there's some deprecation issues with updating products due to the inventory fields. Those should probably be removed or at least handled better for updating products.

For now I've just been dumping it to a map and removing the offending keys so shopify accepts it

Ninigi · 2020-05-29T13:22:15Z

@ProtoJazz @balexand I am a maintainer in this project, and I have to apologize for the lack of attention we have been spending on this library.

Truth is (only speaking for myself here, @nsweeting might have a different oppinion) that the initial approach of having structs for each resource has lost a lot of viability since Shopify started to update their API almost every 4 months.
You would also have to keep updating a library of older structs, that are still compatible with older versions, making the library kind of bulky and hard to understand for its use cases.

Ever since Shopify started using versioned REST API, we have largely switched to using their GraphQL API, and I am seeing the same problems there: If you want to use any Shopify API, you better be ready to fully commit to being a Shopify developer, because they are cranking out new features and deprecations in a pace that I find disturbing. My personal opinion is that Shopify probably thinks of Shopify-App developers as basically their employees, and we have to keep up with what they dictate, but I think they are just violating the basic idea of what an API is: A lasting contract between consumer and supplier.

I would like to contribute more, but I simply can not. There is no time management in the world that could give me the freedom to give you guys the attention you deserve.

I spent days basically going through Shopify's REST API documentation and implementing the missing resources, trying to accommodate for their ideas in how a REST API works, coming up with half backed solutions to how we could solve it. I learned a lot, it was my entry into Elixir open-source and I would not want to miss it - but I certainly do not want (or can afford) to spend the rest of my developer life chasing Shopify's new inventions.

@nsweeting by all means, I think it is time to pass the torch, find other maintainers or maybe transfer the whole project.

ProtoJazz · 2020-05-29T14:39:32Z

Yeah, I suspect as shopify grows they're finding certain parts of their API just not sustainable at scale. They just grow at such a crazy pace.

Thankfully after the change to the versioned api, at least theres some warning about changes and time to fix it

I wouldn't be opposed to helping as a maintainer. I'm still pretty new to elixir, but I've been working with shopify everyday for the last few years and don't see any signs of stopping any time soon

Ninigi · 2020-05-29T13:37:30Z

lib/shopify/enumerable.ex

+  @enforce_keys [:data, :func, :middleware, :params, :session]
+  defstruct @enforce_keys


Is there a reason to @enforce_keys if the enforced keys are the struct keys?

@enforce_keys makes it so that it is an error to instantiate the struct without all of the fields. It has the potential to prevent bugs where a field is accidentally omitted.

It's optional and I can remove it if you'd like.

No, I like the idea.

lib/shopify/enumerable.ex

Ninigi · 2020-05-29T13:43:22Z

lib/shopify/enumerable.ex

+      end
+    end
+
+    def reduce(%Shopify.Enumerable{data: [h | t]} = e, {:cont, acc}, fun) do


Please don't use h t for head and tail. what is the head? I literally can not even give advice on what it should be, because I don't know what h is.

I'll rename these variables to head and tail. Heads up that pattern matching using the names [h | t] and [head | tail] is a common pattern in Elixir. You can find many examples of both naming conventions in the Elixir source code, like https://github.com/elixir-lang/elixir/blob/b9209ad1ac9c879d7ea17c635a14b22aebbf2242/lib/elixir/lib/enum.ex#L2126-L2128

h and t are pretty standard. I dont think naming them head and tail gives any more context on what the data actually is

You are both right, and I am sorry for being unclear. I am talking from a code reading point of view. Sure, I know h and t but what the hell is it in this case!

I still hate on t and h even if there is no way to describe them further, you know why? try to select all t and h in your file and change them to tail and head - you know what I am talking about.

balexand · 2020-05-29T15:48:27Z

Hi @Ninigi. Thanks for your response.

I can definitely relate to your difficulties with the Shopify API. It has lots of quirks and inconsistencies and they do make breaking changes at an alarming rate. I work with the Shopify API constantly at my current job so I really hope things stabilize.

I would prefer to become a regular contributor to this project as opposed to completely taking it over. This could mean giving me access to merge pull-requests in the Github repo. Or it could mean that you would recognize me as a trusted contributor and be willing to merge my pull-requests with less review. In either case, I would consult with the other project members before making any significant changes.

@Ninigi and @nsweeting, would you be interested in having me become a more regular contributor? Also, I'm curious to know how involved you both are interested in being with this project. We can discuss here or I can do a Zoom call if that works better for you.

balexand · 2020-05-29T16:06:28Z

What do you think of removing the middleware option? It complicates the API a bit and I've since moved towards using a custom adapter to throttle requests in my app. Here's the description of the middleware option:

shopify/lib/shopify/pagination.ex

Lines 31 to 35 in bd6884d

    
               * `:middleware` - An optional middleware function whose primary purpose is to allow an 
        
                 application to throttle requests to avoid going over Shopify's rate limit. Must be a 
        
                 function with an arity of 1 that will be passed another function. The middleware must call 
        
                 that function and return the value returned by that function, which will be `{:ok, 
        
                 %Shopify.Response{}}` or `{:error, %Shopify.Error{}}`. Defaults to `& &1.()`.

larskluge · 2020-06-01T10:37:42Z

lib/shopify/enumerable.ex

+            fun
+          )
+
+        {:error, %Error{} = error} ->


Here is a 3rd case that needs to be considered:

{:error, %Response{}} e.g. Shopify fails (as the API does this relatively often) with a 500. Perhaps a retry in this case would be the best thing to do as paginating over a large collection of items can abort with high cost to re-iterate the whole collection.

Thanks, I'll look into this. I have a busy week ahead of me so there's a chance I won't get to it until next Monday.

hashim-sohail · 2020-06-24T11:05:36Z

@balexand @ProtoJazz
Can't find the documentation to use the change. Can you help me?

ProtoJazz · 2020-06-24T13:54:42Z

@balexand @ProtoJazz
Can't find the documentation to use the change. Can you help me?

It hasn't been merged in. You could point at the fork this PR is based on in your mix file https://hexdocs.pm/mix/Mix.Tasks.Deps.html

hashim-sohail · 2020-06-24T13:56:34Z

@balexand @ProtoJazz
Can't find the documentation to use the change. Can you help me?

It hasn't been merged in. You could point at the fork this PR is based on in your mix file https://hexdocs.pm/mix/Mix.Tasks.Deps.html

I have pointed to it, was looking for docs in how to use the new pagination

Shopify.Pagination module

8efb16e

Ninigi reviewed May 29, 2020

View reviewed changes

balexand added 2 commits May 29, 2020 09:59

rename variable e to enum

1d4442b

rename variable h to head and t to tail

bd6884d

larskluge reviewed Jun 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shopify.Pagination module #92

Shopify.Pagination module #92

balexand commented Dec 18, 2019

balexand commented Jan 1, 2020 •

edited

Loading

larskluge commented Mar 6, 2020

balexand commented Mar 6, 2020

ProtoJazz commented May 28, 2020

Ninigi commented May 29, 2020

ProtoJazz commented May 29, 2020 •

edited

Loading

Ninigi May 29, 2020

balexand May 29, 2020

Ninigi Jul 17, 2020

Ninigi May 29, 2020

balexand May 29, 2020

ProtoJazz May 29, 2020

Ninigi Jul 17, 2020

balexand commented May 29, 2020

balexand commented May 29, 2020

larskluge Jun 1, 2020

balexand Jun 1, 2020 •

edited

Loading

hashim-sohail commented Jun 24, 2020

ProtoJazz commented Jun 24, 2020

hashim-sohail commented Jun 24, 2020 •

edited

Loading

		@enforce_keys [:data, :func, :middleware, :params, :session]
		defstruct @enforce_keys

Shopify.Pagination module #92

Are you sure you want to change the base?

Shopify.Pagination module #92

Conversation

balexand commented Dec 18, 2019

balexand commented Jan 1, 2020 • edited Loading

larskluge commented Mar 6, 2020

balexand commented Mar 6, 2020

ProtoJazz commented May 28, 2020

Ninigi commented May 29, 2020

ProtoJazz commented May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balexand commented May 29, 2020

balexand commented May 29, 2020

Choose a reason for hiding this comment

balexand Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

hashim-sohail commented Jun 24, 2020

ProtoJazz commented Jun 24, 2020

hashim-sohail commented Jun 24, 2020 • edited Loading

balexand commented Jan 1, 2020 •

edited

Loading

ProtoJazz commented May 29, 2020 •

edited

Loading

balexand Jun 1, 2020 •

edited

Loading

hashim-sohail commented Jun 24, 2020 •

edited

Loading