-
-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add support for custom cache stores (options.read/options.write
)
#19
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would read/write from e.g a database be faster then fs
?
What 'real' use cases does this offer to users?
I'm not 💯 convinced if this is actually needed...
README.md
Outdated
@@ -49,6 +49,8 @@ module.exports = { | |||
|:--:|:--:|:-----:|:----------| | |||
|**`cacheDirectory`**|`{String}`|`path.resolve('.cache-loader')`|Provide a cache directory where cache items should be stored| | |||
|**`cacheIdentifier`**|`{String}`|`cache-loader:{version} {process.env.NODE_ENV}`|Provide an invalidation identifier which is used to generate the hashes. You can use it for extra dependencies of loaders.| | |||
|**`writeCache`**|`{Function(cacheFilename, content, callback) -> {void}}`|`undefined`|Allows you to override default write cache data to file (e.g. Redis, memcached).| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just writer
(options.writer
) ?
README.md
Outdated
@@ -49,6 +49,8 @@ module.exports = { | |||
|:--:|:--:|:-----:|:----------| | |||
|**`cacheDirectory`**|`{String}`|`path.resolve('.cache-loader')`|Provide a cache directory where cache items should be stored| | |||
|**`cacheIdentifier`**|`{String}`|`cache-loader:{version} {process.env.NODE_ENV}`|Provide an invalidation identifier which is used to generate the hashes. You can use it for extra dependencies of loaders.| | |||
|**`writeCache`**|`{Function(cacheFilename, content, callback) -> {void}}`|`undefined`|Allows you to override default write cache data to file (e.g. Redis, memcached).| | |||
|**`readCache`**|`{Function(cacheFilename, callback) -> {void}}`|`undefined`|Allows you to override default read cache data from file.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reader
(options.reader
) ?
README.md
Outdated
@@ -49,6 +49,8 @@ module.exports = { | |||
|:--:|:--:|:-----:|:----------| | |||
|**`cacheDirectory`**|`{String}`|`path.resolve('.cache-loader')`|Provide a cache directory where cache items should be stored| | |||
|**`cacheIdentifier`**|`{String}`|`cache-loader:{version} {process.env.NODE_ENV}`|Provide an invalidation identifier which is used to generate the hashes. You can use it for extra dependencies of loaders.| | |||
|**`writeCache`**|`{Function(cacheFilename, content, callback) -> {void}}`|`undefined`|Allows you to override default write cache data to file (e.g. Redis, memcached).| | |||
|**`readCache`**|`{Function(cacheFilename, callback) -> {void}}`|`undefined`|Allows you to override default read cache data from file.| | |||
|
|||
<h2 align="center">Examples</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an example aswell
@michael-ciniawsky I have changed option names and added example from my first post. |
README.md
Outdated
@@ -95,6 +97,62 @@ module.exports = { | |||
} | |||
``` | |||
|
|||
### `Write cache data to redis` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please try to make this example a bit more generic? e.g Database Intergation
something like that :)
options.reader/options.writer
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- rename
reader
->read
,writer
->write
- move the JSON.stringify and JSON.parse logic into the
read
andwrite
methods. - rename
cacheFilename
tocacheKey
- add a
makeKey
functionmakeKey(options, request)
- default
makeKey
=path.join(..., digest(cacheIdentifer + request) + .json
cache-loader should no longer talk about filenames. Filenames should only occur in the default read/write/key implementation. |
options.reader/options.writer
)options.read/options.write
)
20d9c2d
to
3afabf8
Compare
@sokra refactored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is options.makeKey
mandatory functionality or additional ? It's better named options.cacheKey
for naming consistency with options.cacheDirectory
,options.cacheIdentfier
README.md
Outdated
@@ -95,6 +98,82 @@ module.exports = { | |||
} | |||
``` | |||
|
|||
### `Example of Database Integration` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Database Intergration
src/index.js
Outdated
@@ -9,7 +9,18 @@ const pkgVersion = require('../package.json').version; | |||
const defaultCacheDirectory = path.resolve('.cache-loader'); | |||
const ENV = process.env.NODE_ENV || 'development'; | |||
|
|||
const defaultOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultOptions
=> defaults
src/index.js
Outdated
@@ -9,7 +9,18 @@ const pkgVersion = require('../package.json').version; | |||
const defaultCacheDirectory = path.resolve('.cache-loader'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- const defaultCacheDirectory = path.resolve('.cache-loader');
src/index.js
Outdated
@@ -9,7 +9,18 @@ const pkgVersion = require('../package.json').version; | |||
const defaultCacheDirectory = path.resolve('.cache-loader'); | |||
const ENV = process.env.NODE_ENV || 'development'; | |||
|
|||
const defaultOptions = { | |||
cacheDirectory: defaultCacheDirectory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheDirectory: path.resolve('.cache-loader'),
src/index.js
Outdated
function loader(...args) { | ||
const loaderOptions = loaderUtils.getOptions(this) || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- const loaderOptions = loaderUtils.getOptions(this) || {};
src/index.js
Outdated
function write(cacheKey, data, callback) { | ||
const dirname = path.dirname(cacheKey); | ||
const content = JSON.stringify(data); | ||
if (directoriesExists[dirname]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (directories.has(dirname)) {
src/index.js
Outdated
callback(mkdirErr); | ||
return; | ||
} | ||
directoriesExists[dirname] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\ndirectories.add(dirname)
src/index.js
Outdated
}); | ||
} | ||
|
||
function makeKey(options, request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function cacheKey(options, request) {
src/index.js
Outdated
|
||
function makeKey(options, request) { | ||
const { cacheIdentifier, cacheDirectory } = options; | ||
const hash = digest(`${cacheIdentifier}\n${request}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
src/index.js
Outdated
const options = Object.assign({}, defaultOptions, loaderOptions); | ||
const { cacheIdentifier, cacheDirectory } = options; | ||
const { read: readFn, makeKey: makeKeyFn } = options; | ||
const data = dataInput; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to L67
Hello
It would be nice to know if module changed by checking hash content instead of mtime. It would very nice to provide an api to set Can it be done in this PR? |
@michael-ciniawsky @Crazymax11 We have different strategy. Instead of directly cloning repo in the docker image (which is slow), we have cloned repository on the build machine. During build we update this repo with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final nits 🐦 and g2g :)
README.md
Outdated
|**`cacheIdentifier`**|`{String}`|`cache-loader:{version} {process.env.NODE_ENV}`|Provide an invalidation identifier which is used to generate the hashes. You can use it for extra dependencies of loaders.| | ||
|**`cacheDirectory`**|`{String}`|`path.resolve('.cache-loader')`|Provide a cache directory where cache items should be stored (used for default read/write implementation)| | ||
|**`cacheIdentifier`**|`{String}`|`cache-loader:{version} {process.env.NODE_ENV}`|Provide an invalidation identifier which is used to generate the hashes. You can use it for extra dependencies of loaders. (used for default read/write implementation)| | ||
|**`cacheKey`**|`{Function(options, request) -> {string}}`|`undefined`|Allows you to override default cache key generator.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please finally move this to be the first item in the options table (before cacheDirectory
) && -> {string}
=> -> {String}
src/index.js
Outdated
}; | ||
const options = Object.assign({}, defaultOptions, loaderOptions); | ||
const { cacheIdentifier, cacheDirectory } = options; | ||
const options = Object.assign({}, defaults, loaderUtils.getOptions(this)); | ||
const data = dataInput; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- const data = dataInput
return; | ||
} | ||
|
||
directories.add(dirname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
|
||
function cacheKey(options, request) { | ||
const { cacheIdentifier, cacheDirectory } = options; | ||
const hash = digest(`${cacheIdentifier}\n${request}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
src/index.js
Outdated
const hash = digest(`${cacheIdentifier}\n${remainingRequest}`); | ||
const cacheFile = path.join(cacheDirectory, `${hash}.json`); | ||
const { read: readFn, cacheKey: cacheKeyFn } = options; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = dataInput
data.cacheKey = cacheKeyFn(options, remainingRequest)
data.remainingRequest = remainingRequest
@michael-ciniawsky Changed. Can I ask you why are these |
Of course, Code Readability. I know it is annoying and doesn't add anything, but that code needs to be maintained and whitespace in particular is one of the cheapest ways to improve readability.
Yep but it definitely should have and if no one is pesky about it the tendency is to become one big 'BLOB' again. JSDoc is also missing 😛 |
Anything left TODO left here ? @sokra @webpack-contrib/org-maintainers please review when time :) |
Looks like @Mordred has completed all the review comments. I don't personally see any other issues that stand out. Unless Tobias has an objection, I'd say let it rip. :) |
LGTM. Although... I know this applies more to this project as a whole - would be nice to see some tests. Not really practical to hold this up for that reason, though. |
@hulkish - @michael-ciniawsky setup a base test configuration & we are working through the repos that don't already have suites. Just haven't gotten here yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
options.read/options.write
)options.read/options.write
)
Hi, I want to change how cache data are stored in my project. In my case I want to use Redis server like this:
Then in my webpack config:
For that I needed
writeCache
andreadCache
hooks.