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

Add redis cache set and get example docs #1254

Closed
wants to merge 3 commits into from

Conversation

SOHELAHMED7
Copy link

@SOHELAHMED7 SOHELAHMED7 commented May 19, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[x] Other... Please describe: docs enhancement

Added Redis cache simple set and get example

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Feedback appreciated.

@SOHELAHMED7
Copy link
Author

#1252

@@ -161,6 +161,42 @@ import { AppController } from './app.controller';
export class ApplicationModule {}
```

##### Redis cache `set` and `get` example
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
##### Redis cache `set` and `get` example
#### Redis cache `set` and `get` example

Copy link
Author

Choose a reason for hiding this comment

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

fixed in 9c713df

@@ -161,6 +161,42 @@ import { AppController } from './app.controller';
export class ApplicationModule {}
```

##### Redis cache `set` and `get` example

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I don't understand what this example is trying to do, or how the code is working, without some greater context. Could you please try to explain:

  1. What is the goal of this section
  2. What are the relevant portions of the API shown in the example and any concepts needed to understand them (i.e., this is the first time we're seeing CacheManager, so we need some context to understand how it works).

Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

This section adds simple example how to use cache (e.g. Redis cache) in the code. The current documention is nice but does not include any example or docs about how to simply set "values" by "keys" in Redis cache and get it.

Like for e.g

Yii framework's docs

// try retrieving $data from cache
$data = $cache->get($key);

if ($data === false) {
    // $data is not found in cache, calculate it from scratch
    $data = $this->calculateSomething();

    // store $data in cache so that it can be retrieved next time
    $cache->set($key, $data);
}

// $data is available here

Laravel's Docs

Cache::put('key', 'value', $seconds);

$value = Cache::get('key');

CacheManager provide generic API regardless of what is used to store cache e.g. Redis, MySQL, file etc.

Feel free to let me know if more info needed

});

// async/await way
await this.cacheManager.set('foo2', 'bar2', {ttl: 100000});

Choose a reason for hiding this comment

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

It might be a good practice to wrap this in a try/catch block.

Copy link
Author

Choose a reason for hiding this comment

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

Agree. Just for docs it is kept like this and trying to keep minimal. I checked other part of docs, not at all place await statement are in try-catch blocks. E.g. https://docs.nestjs.com/pipes#class-validator

Choose a reason for hiding this comment

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

Alright, makes sense.

@kamilmysliwiec
Copy link
Member

@kamilmysliwiec
Copy link
Member

We just added a dedication section making this PR obsolete. Thanks for your contribution though!

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

Successfully merging this pull request may close these issues.

None yet

4 participants