feat: add support for custom cache stores (`options.read/options.write`) #19

Mordred · 2017-09-27T14:00:48Z

Hi, I want to change how cache data are stored in my project. In my case I want to use Redis server like this:

const redis = require('redis');

// ...
// ... connect to client
// ...

const BUILD_CACHE_TIMEOUT = 24 * 3600; // 1 day

function redisKey(filename) {
    return `build:cache:${filename}`;
}

function readCache(filename, callback) {
      client.get(redisKey(filename), (err, result) => {
          if (err) {
              return callback(err);
          }

          if (!result) {
              return callback(new Error(`Key ${redisKey(filename)} not found`));
          }

          callback(null, result);
    });
}

function writeCache(filename, content, callback) {
      client.set(redisKey(filename), content, 'EX', BUILD_CACHE_TIMEOUT, callback);
}

Then in my webpack config:

{
  loader: 'cache-loader',
  options: {
    readCache,
    writeCache,
  },
},

For that I needed writeCache and readCache hooks.

jsf-clabot · 2017-09-27T14:00:49Z

All committers have signed the CLA.

alexander-akait

Looks good, thank you!

michael-ciniawsky

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...

michael-ciniawsky · 2017-09-28T15:23:50Z

README.md

@@ -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).|


Just writer (options.writer) ?

michael-ciniawsky · 2017-09-28T15:24:12Z

README.md

@@ -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.|


Just reader (options.reader) ?

michael-ciniawsky · 2017-09-28T15:31:21Z

README.md

@@ -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>


Please add an example aswell

Mordred · 2017-09-29T07:18:23Z

@michael-ciniawsky fs is faster than database, but we are building application for production in the docker. Saving cached data to filesystem is not an option because mounting volumes during docker build is not possible.

I have changed option names and added example from my first post.

michael-ciniawsky · 2017-10-19T14:58:26Z

README.md

@@ -95,6 +97,62 @@ module.exports = {
 }
 ```

+### `Write cache data to redis`


Could you please try to make this example a bit more generic? e.g Database Intergation something like that :)

sokra

rename reader -> read, writer -> write
move the JSON.stringify and JSON.parse logic into the read and write methods.
rename cacheFilename to cacheKey
add a makeKey function makeKey(options, request)
default makeKey = path.join(..., digest(cacheIdentifer + request) + .json

sokra · 2017-10-19T15:57:52Z

cache-loader should no longer talk about filenames. Filenames should only occur in the default read/write/key implementation.

Mordred · 2017-10-23T09:02:40Z

@sokra refactored

michael-ciniawsky

Is options.makeKey mandatory functionality or additional ? It's better named options.cacheKey for naming consistency with options.cacheDirectory,options.cacheIdentfier

michael-ciniawsky · 2017-10-23T13:21:28Z

README.md

@@ -95,6 +98,82 @@ module.exports = {
 }
 ```

+### `Example of Database Integration`


### Database Intergration

michael-ciniawsky · 2017-10-23T13:23:41Z

src/index.js

@@ -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 = {


defaultOptions => defaults

michael-ciniawsky · 2017-10-23T13:24:13Z

src/index.js

@@ -9,7 +9,18 @@ const pkgVersion = require('../package.json').version;
 const defaultCacheDirectory = path.resolve('.cache-loader');


- const defaultCacheDirectory = path.resolve('.cache-loader');

michael-ciniawsky · 2017-10-23T13:24:57Z

src/index.js

@@ -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,


cacheDirectory: path.resolve('.cache-loader'),

michael-ciniawsky · 2017-10-23T13:25:39Z

src/index.js

 function loader(...args) {
+  const loaderOptions = loaderUtils.getOptions(this) || {};


- const loaderOptions = loaderUtils.getOptions(this) || {};

michael-ciniawsky · 2017-10-23T13:32:34Z

src/index.js

+function write(cacheKey, data, callback) {
+  const dirname = path.dirname(cacheKey);
+  const content = JSON.stringify(data);
+  if (directoriesExists[dirname]) {


if (directories.has(dirname)) {

michael-ciniawsky · 2017-10-23T13:34:37Z

src/index.js

+        callback(mkdirErr);
+        return;
+      }
+      directoriesExists[dirname] = true;


\ndirectories.add(dirname)

michael-ciniawsky · 2017-10-23T13:35:19Z

src/index.js

+  });
+}
+
+function makeKey(options, request) {


function cacheKey(options, request) {

michael-ciniawsky · 2017-10-23T13:35:30Z

src/index.js

+
+function makeKey(options, request) {
+  const { cacheIdentifier, cacheDirectory } = options;
+  const hash = digest(`${cacheIdentifier}\n${request}`);


michael-ciniawsky · 2017-10-23T13:37:32Z

src/index.js

  const options = Object.assign({}, defaultOptions, loaderOptions);
-  const { cacheIdentifier, cacheDirectory } = options;
+  const { read: readFn, makeKey: makeKeyFn } = options;
  const data = dataInput;


Please move this to L67

Crazymax11 · 2017-10-24T08:15:54Z

Hello
We also tried to use cache loader to build application in docker and met two problems:

Cache loader uses mtime to check if dependency changed, but git doesn't store files mtime, so after git clone we always have new mtime and no cache used.
It seems that cache loader doesn't use content of resource to check if resources changed, cache loader just checks if dependencies changed.

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 checkIfChanged function as option.

Can it be done in this PR?

Mordred · 2017-10-24T12:03:32Z

@michael-ciniawsky makeKey -> cacheKey

@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 git pull and copy it into docker context with COPY.

michael-ciniawsky

Final nits 🐦 and g2g :)

michael-ciniawsky · 2017-10-24T16:09:26Z

README.md

-|**`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.|


Please finally move this to be the first item in the options table (before cacheDirectory) && -> {string} => -> {String}

michael-ciniawsky · 2017-10-24T16:10:22Z

src/index.js

-  };
-  const options = Object.assign({}, defaultOptions, loaderOptions);
-  const { cacheIdentifier, cacheDirectory } = options;
+  const options = Object.assign({}, defaults, loaderUtils.getOptions(this));
  const data = dataInput;


- const data = dataInput

#19 (comment)

michael-ciniawsky · 2017-10-24T16:12:44Z

src/index.js

+        return;
+      }
+
+      directories.add(dirname);


michael-ciniawsky · 2017-10-24T16:12:54Z

src/index.js

+
+function cacheKey(options, request) {
+  const { cacheIdentifier, cacheDirectory } = options;
+  const hash = digest(`${cacheIdentifier}\n${request}`);


michael-ciniawsky · 2017-10-24T16:16:10Z

src/index.js

-  const hash = digest(`${cacheIdentifier}\n${remainingRequest}`);
-  const cacheFile = path.join(cacheDirectory, `${hash}.json`);
+  const { read: readFn, cacheKey: cacheKeyFn } = options;
+


const data = dataInput data.cacheKey = cacheKeyFn(options, remainingRequest) data.remainingRequest = remainingRequest

Mordred · 2017-10-24T18:14:06Z

@michael-ciniawsky Changed.

Can I ask you why are these \n changes so important? Current index.js has no newlines in function definitions and I tried to match that style :)

michael-ciniawsky · 2017-10-24T18:34:00Z

Can I ask you why are these \n changes so important?

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.

Current index.js has no newlines in function definitions and I tried to match that style :)

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 😛

michael-ciniawsky · 2017-10-27T22:52:34Z

Anything left TODO left here ? @sokra @webpack-contrib/org-maintainers please review when time :)

joshwiens · 2017-10-28T03:20:32Z

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. :)

hulkish · 2017-10-28T03:36:46Z

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.

joshwiens · 2017-10-28T03:44:11Z

@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.

sokra

LGTM

alexander-akait requested review from sokra, alexander-akait and michael-ciniawsky September 28, 2017 09:15

alexander-akait added priority: 5 (nice to have) semver: Minor type: Feature labels Sep 28, 2017

alexander-akait approved these changes Sep 28, 2017

View reviewed changes

michael-ciniawsky reviewed Sep 28, 2017

View reviewed changes

Mordred force-pushed the own-cache branch from fe4bcfd to d1cb832 Compare September 29, 2017 07:06

michael-ciniawsky suggested changes Oct 19, 2017

View reviewed changes

michael-ciniawsky changed the title ~~feat: Allow to change how the cache is stored~~ feat: add support for custom cache store mechanisms (options.reader/options.writer) Oct 19, 2017

sokra suggested changes Oct 19, 2017

View reviewed changes

michael-ciniawsky changed the title ~~feat: add support for custom cache store mechanisms (options.reader/options.writer)~~ feat: add support for custom cache store mechanisms (options.read/options.write) Oct 19, 2017

Mordred force-pushed the own-cache branch 2 times, most recently from 20d9c2d to 3afabf8 Compare October 23, 2017 08:55

michael-ciniawsky suggested changes Oct 23, 2017

View reviewed changes

Mordred force-pushed the own-cache branch from 3afabf8 to f660777 Compare October 24, 2017 11:56

michael-ciniawsky approved these changes Oct 24, 2017

View reviewed changes

michael-ciniawsky requested review from sokra and alexander-akait October 24, 2017 16:13

michael-ciniawsky reviewed Oct 24, 2017

View reviewed changes

michael-ciniawsky added this to the 1.1.0 milestone Oct 24, 2017

michael-ciniawsky added pr_action: Review status: Pending labels Oct 24, 2017

michael-ciniawsky removed the priority: 5 (nice to have) label Oct 24, 2017

feat: Allow to change how cache is stored

7a879b7

Mordred force-pushed the own-cache branch from f660777 to 7a879b7 Compare October 24, 2017 18:05

joshwiens approved these changes Oct 28, 2017

View reviewed changes

sokra approved these changes Nov 15, 2017

View reviewed changes

michael-ciniawsky changed the title ~~feat: add support for custom cache store mechanisms (options.read/options.write)~~ feat: add support for custom cache stores (options.read/options.write) Nov 15, 2017

michael-ciniawsky added status: Approved and removed pr_action: Review status: Pending labels Nov 15, 2017

michael-ciniawsky merged commit 060796b into webpack-contrib:master Nov 15, 2017

michael-ciniawsky mentioned this pull request Nov 16, 2017

refactor(loader): v2.0.0 #25

Closed

michael-ciniawsky removed this from the 1.3.0 milestone Feb 26, 2018

		@@ -9,7 +9,18 @@ const pkgVersion = require('../package.json').version;
		const defaultCacheDirectory = path.resolve('.cache-loader');

		function loader(...args) {
		const loaderOptions = loaderUtils.getOptions(this) \|\| {};

feat: add support for custom cache stores (options.read/options.write) #19

feat: add support for custom cache stores (options.read/options.write) #19

Conversation

Mordred commented Sep 27, 2017

jsf-clabot commented Sep 27, 2017 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

michael-ciniawsky left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mordred commented Sep 29, 2017

Choose a reason for hiding this comment

sokra left a comment

Choose a reason for hiding this comment

sokra commented Oct 19, 2017

Mordred commented Oct 23, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crazymax11 commented Oct 24, 2017

Mordred commented Oct 24, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mordred commented Oct 24, 2017

michael-ciniawsky commented Oct 24, 2017

michael-ciniawsky commented Oct 27, 2017

joshwiens commented Oct 28, 2017

hulkish commented Oct 28, 2017

joshwiens commented Oct 28, 2017

sokra left a comment

Choose a reason for hiding this comment

feat: add support for custom cache stores (`options.read/options.write`) #19

feat: add support for custom cache stores (`options.read/options.write`) #19

jsf-clabot commented Sep 27, 2017 •

edited

michael-ciniawsky left a comment •

edited

michael-ciniawsky Oct 24, 2017 •

edited