Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impossible to set Redis config #22

Closed
Ilshidur opened this issue Jul 29, 2017 · 6 comments
Closed

Impossible to set Redis config #22

Ilshidur opened this issue Jul 29, 2017 · 6 comments
Labels

Comments

@Ilshidur
Copy link
Contributor

Ilshidur commented Jul 29, 2017

First of all, I really like your lib. Keep up the good work !

I'm facing issues when trying to set the Redis cache options.
I got this :

this.api = new KindredApi.Kindred({
  key: apiKey,
  defaultRegion: region,
  debug: !prod,
  showKey: true,
  showHeaders: false,
  limits: prod ? KindredApi.LIMITS.PROD : KindredApi.LIMITS.DEV,
  spread: false,
  retryOptions: {
    auto: true,
    numberOfRetriesBeforeBreak: 3,
  },
  timeout: 5000,
  cache: new KindredApi.RedisCache({ // Problem here, I can't set the options
    host: cache.redis.host || 'localhost',
    port: cache.redis.port || 6379,
    keyPrefix: cache.redis.prefix || 'league-tooltips_',
  }),
  cacheTTL: {
    STATIC: cache.TTL,
  },
});

The problem is that it is impossible to set the Redis config I want. They always get overridden by your default config. See here. Your code behaves this way : whatever the user gives, the default options are overriding it.

The way to fix this is to change the line to this :

var options = Object.assign({
  host: '127.0.0.1',
  port: 6379,
  keyPrefix: 'kindredAPI-'
}, opts || {});

The second problem is that the Wiki needs more docs about the RedisCache options. I had to dig in the lib code.

If you don't have the time to fix this, I can send you a PR with the fixed code :)

@Ilshidur
Copy link
Contributor Author

Submitted a pull request.

@cnguy
Copy link
Owner

cnguy commented Jul 29, 2017

@Ilshidur

Hey, sorry about that actually. I completely forgot about the Redis options even though I knew it was a problem (I always had my redis stuffs on default so I forgot about it being an issue). I'll check out your PR.

@Ilshidur
Copy link
Contributor Author

No problem ! I can also help you to write the RedisCache docs if necessary ;-)

cnguy pushed a commit that referenced this issue Jul 29, 2017
* Fix (#22) : Redis config could not be set in the options

This PR solves the problem of the issue #22.

* Removed extra semicolon
@cnguy
Copy link
Owner

cnguy commented Jul 29, 2017

@Ilshidur

If you want to, I'd appreciate it. If anything, I'll write it by the end of the day.

@cnguy
Copy link
Owner

cnguy commented Jul 29, 2017

@Ilshidur To run the tests, also set KEY_TO_RATE_LIMIT to your dev key (there's a test that mocks the 429 retries until successful). I'll add this to docs as well (Contributing?).

edit: I also have to update this test now that the rate limit has changed.

published to 2.0.74

Updated Wiki Docs to show current Redis options (https://github.com/ChauTNguyen/kindred-api/wiki/Caching#redis-cache-example) and also updated the README to have a small configurable Redis example in the README Introduction section.

@Ilshidur
Copy link
Contributor Author

Awesome ! Closing the issue, since you solved it :)

@cnguy cnguy added the bug label Jul 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants