Skip to content
This repository has been archived by the owner on May 3, 2021. It is now read-only.

Add optional cache layer to help with (jquery.instagramFeed) issue #25. Default state is not caching. #52

Closed
wants to merge 11 commits into from

Conversation

cube-dan
Copy link

Greets Team InstagramFeed,

I've added a cache layer to help deal with this error message:

InstagramFeed: It looks like YOUR network has been temporary banned because of too many requests. See https://github.com/jsanahuja/jquery.instagramFeed/issues/25

This adds 3 new config options:

   // option.name: default_value
   cache_use: false,
   cache_for: "30:min",
   cache_prefix: "igf_",

Docs & version number updated as per CONTRIBUTING.md.

An aside: I ran into a small snafu with the version number. There's a mismatch between src/
InstagramFeed.js (@1.4.0) and package.json (@1.5.4). I ended up going with 1.6.0 since this is a backwards compatible API change.

Let me know if you'd like anything changed and thank you SO much for an Instagram option that isn’t painful!

-Dan

@cube-dan cube-dan changed the title Add optional cache layer to help with issue #25. Default state is not caching. Add optional cache layer to help with issue #25 (jquery.instagramFeed). Default state is not caching. Feb 15, 2021
@cube-dan cube-dan changed the title Add optional cache layer to help with issue #25 (jquery.instagramFeed). Default state is not caching. Add optional cache layer to help with (jquery.instagramFeed) issue #25. Default state is not caching. Feb 15, 2021
@cube-dan
Copy link
Author

One more thing team InstagramFeed. If you're cool with adding this PR — and after fixing anything you guys want me to — I'll go ahead and add it to jquery.instagramFeed also.

@jsanahuja
Copy link
Owner

jsanahuja commented Feb 17, 2021

As it is stated in the CONTRIBUTING.md file:

When contributing to this repository, please first discuss the change you wish to make via issue, email, or any other method with the owners of this repository before making a change.

I am sorry but the implementation of the client-side catching should follow what was described in #49

Copy link
Owner

@jsanahuja jsanahuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be implemented as it was done in https://github.com/jsanahuja/jquery.instagramFeed/pull/97/files
I don't think we need 3 parameters nor "complex" time specification for this

@cube-dan
Copy link
Author

I'm not sure what you’re asking for here @jsanahuja. #49 seems to imply that queries should be made through the IG graphql url instead of how you guys are doing it now; scraping returned html.

I'm also confused about what you mean by:

Should be implemented as it was done in https://github.com/jsanahuja/jquery.instagramFeed/pull/97/files

as the url you referenced show code that isn’t present in the current version of InstagramFeed. Should I have started the PR from a different branch?

My thoughts on the "complex" time specification come down to the idea of making the API easy on the end user. I agree that 3 variables could be reduced but I didn’t want to be presumptuous by giving the end user no way of turning the caching off, nor assuming that one caching time would fit all.

I agree with what you said in #97:

I don't really know, depends on how often does people publish on their Instagram accounts.

... the cache time needs to be up to the end user.

I also agree that the default caching state should be "ON"

What would you recommend as a solution, from an end user perspective, that would allow:

  1. Easy to "think about" cache time periods
  2. Cache on by default

Perhaps cache_for: false would turn off caching?

@jsanahuja
Copy link
Owner

jsanahuja commented Feb 22, 2021

I'm not sure what you’re asking for here @jsanahuja. #49 seems to imply that queries should be made through the IG graphql url instead of how you guys are doing it now; scraping returned html.

I'm also confused about what you mean by:

Should be implemented as it was done in https://github.com/jsanahuja/jquery.instagramFeed/pull/97/files

as the url you referenced show code that isn’t present in the current version of InstagramFeed. Should I have started the PR from a different branch?

My thoughts on the "complex" time specification come down to the idea of making the API easy on the end user. I agree that 3 variables could be reduced but I didn’t want to be presumptuous by giving the end user no way of turning the caching off, nor assuming that one caching time would fit all.

I agree with what you said in #97:

I don't really know, depends on how often does people publish on their Instagram accounts.

... the cache time needs to be up to the end user.

I also agree that the default caching state should be "ON"

What would you recommend as a solution, from an end user perspective, that would allow:

  1. Easy to "think about" cache time periods
  2. Cache on by default

Perhaps cache_for: false would turn off caching?

jquery.instagramFeed is the jQuery version of this repository. We do the same on both repositories so the cache thing should be implemented as it was done there in https://github.com/jsanahuja/jquery.instagramFeed/pull/97/files. You can use different ways but the end user API (and default values) must be the same

Thank you :)

@jsanahuja
Copy link
Owner

jsanahuja commented Feb 22, 2021

I am closing this PR as this feature has been implemented in #54

@jsanahuja jsanahuja closed this Feb 22, 2021
@cube-dan
Copy link
Author

Thanks for considering the change @jsanahuja. Glad you guys already had caching in the pipe!

I noticed that you guys rolled out #54 with 2.0.2 and that it's live on packagist so, again, thanks for getting caching implemented with the new cache_time var.

@jsanahuja
Copy link
Owner

jsanahuja commented Feb 22, 2021

it's live on packagist

I don't know who mantains that. We only publish it in the Github NPM Registry (more info here) and CDNJS

Thank you for your support! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants