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

Feature/cache middleware #80

Merged
merged 6 commits into from Jan 13, 2018
Merged

Feature/cache middleware #80

merged 6 commits into from Jan 13, 2018

Conversation

lmammino
Copy link
Member

Adds a caching middleware

@codecov
Copy link

codecov bot commented Jan 12, 2018

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #80   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     11    +1     
  Lines         166    183   +17     
  Branches       37     38    +1     
=====================================
+ Hits          166    183   +17
Impacted Files Coverage Δ
src/middlewares/index.js 100% <ø> (ø) ⬆️
src/middlewares/cache.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e02a9e6...69ad08b. Read the comment docs.

@NPCRUS
Copy link
Contributor

NPCRUS commented Jan 12, 2018

Correct me if I wrong please. Looks like this is sort of in-memory cache, and in this case we can't really apply such middleware for lambda functions, because containers will have different cache and other problems. Lambda may not save this between executions. It's better to use some sort of redis or memcache in case of using lambda.
So, maybe you should add some sort of notice to this middleware, that it's not for lambda usage

@peterjcaulfield
Copy link
Contributor

peterjcaulfield commented Jan 12, 2018

@NPCRUS I believe @lmammino is intending to leverage container re-use where possible.

@NPCRUS
Copy link
Contributor

NPCRUS commented Jan 12, 2018

@peterjcaulfield Thank you very much for this article, make sense!

@lmammino
Copy link
Member Author

@NPCRUS, if you check the documentation here and the example you can see that the middleware is broadly configurable. You can provide your own functions to calculate cache ids, save items in the cache and get them from the cache. It should be relatively easy to write those functions to adopt a remote caching backend if you need that.

Copy link
Contributor

@dkatavic dkatavic left a comment

Choose a reason for hiding this comment

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

Great stuff. Just have 1 minor comment

return options.getValue(cacheKey)
})
.then((cachedResponse) => {
if (cachedResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change this with if (cachedResponse === undefined) {? . That way we gonna avoid false negatives when cache is returning 0, null or empty string

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point :)

@lmammino lmammino merged commit 09cf110 into master Jan 13, 2018
@lmammino lmammino deleted the feature/cache-middleware branch January 13, 2018 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants