refactor: replace cacache with flat-cache #121

filipesilva · 2017-09-06T10:53:14Z

See #120 for reasoning

`Issues`

Fixes [Refactor] swap cacache with another cache #120

joshwiens · 2017-10-20T21:05:18Z

@evilebottnawi @michael-ciniawsky - As soon as we land #108, this one is up next to be included in the initial 1.0.0 release. The @angular/cli group needs this for a perf issue.

filipesilva · 2017-10-20T22:04:39Z

@d3viant0ne I'll need to update this to work on top of #108.

michael-ciniawsky · 2017-10-20T22:35:38Z

Can someone or I land #108, so we can triage and fix this then. Did someone try/ask for a LICENSE change @cacache repo?

joshwiens · 2017-10-20T23:01:28Z

@michael-ciniawsky - The snapshots in #108 need to be updated before that merges. Outside of that, i'm pretty sure it's ready to go.

RE License change: That gets messy & has lingering legal issues not to mention we would probably have to engage the JSF legal team which I would rather avoid.

We shouldn't be using CC0 libs either for pretty much the same reason as Angular.

michael-ciniawsky · 2017-10-21T03:03:24Z

#108 landed 🎉

michael-ciniawsky · 2017-10-21T03:07:33Z

Are there any alternatives available ? flat-cache uses sync I/O

alexander-akait · 2017-10-21T15:11:00Z

@filipesilva please rebase

filipesilva · 2017-10-22T16:35:11Z

@michael-ciniawsky there's a couple of alternatives, I just searched for cache in npm and went through the list. My criteria was that it looked well supported, reliable, and had a similar API/feature-set. flat-cache seemed to fit the bill. Do you have any other in mind?

@evilebottnawi done.

filipesilva · 2017-10-22T16:49:06Z

I'm looking at the snapshots and there seems to be a problem, still fixing.

filipesilva · 2017-10-22T17:33:06Z

Seems good now 👍

michael-ciniawsky

Can't we wait until the relicensing is done ?
Do we have any basic benchmark data comparing flat-cache to cacache ? I'm worried about flat-cache using sync File I/O

michael-ciniawsky · 2017-10-22T18:22:07Z

src/uglify/cache.js

+
+const cacheId = 'uglifyjs-webpack-plugin';
+
+export default class Cache {


Why do we need a class here ?

const cache = { get (dir, key) {}, put (dir, key, data) {}, list (dir) {}, clear (dir) {} } export default cache

We don't necessarily need a class, no. Something with a similar api was needed for the Jest spies but that's it. I'll change it to an object if it's preferable.

Bear in mind that changing it from Cache to cache necessitated a few other renames to prevent shadowed variables.

Loading the ?whole cache flatCache.load(id, dir) on every method call seems odd or I'm missing something ?

michael-ciniawsky · 2017-10-22T18:22:36Z

src/uglify/index.js

 import findCacheDir from 'find-cache-dir';
 import workerFarm from 'worker-farm';
 import minify from './minify';
+import Cache from './cache';


Cache => cache

michael-ciniawsky · 2017-10-22T18:23:50Z

src/uglify/cache.js

@@ -0,0 +1,23 @@
+import flatCache from 'flat-cache';
+
+const cacheId = 'uglifyjs-webpack-plugin';


cacheId => id

michael-ciniawsky · 2017-10-22T23:07:48Z

src/uglify/cache.js

+
+const id = 'uglifyjs-webpack-plugin';
+
+const cache = {


export default class Cache { constructor (dir) { this.id = 'uglifyjs-webpack-plugin' this.cache = flatCache.load(this.id, dir) } get (key) { return this.cache.getKey(key) }, put (key, data) { this.cache.setKey(key, JSON.stringify(data)) this.cache.save(true) }, list () { return this.cache.all() }, clear (dir) { flatCache.clearAll(dir) } }

Alternative

// cache(dir) export default function (dir) { const id = 'uglifyjs-webpack-plugin' const cache = flatCache.load(id, dir) return { get (key) { return cache.getKey(key) }, put (key, data) { cache.setKey(key, JSON.stringify(data)) cache.save(true) }, list () { return cache.all() }, clear (dir) { flatCache.clearAll(dir) } } }

michael-ciniawsky · 2017-10-22T23:09:26Z

src/uglify/index.js

 import findCacheDir from 'find-cache-dir';
 import workerFarm from 'worker-farm';
 import minify from './minify';
+import cache from './cache';


cache => Cache

michael-ciniawsky · 2017-10-22T23:19:53Z

src/uglify/index.js

-    const { cache, parallel } = options;
-    this.cacheDir = cache === true ? findCacheDir({ name: 'uglifyjs-webpack-plugin' }) : cache;
+    const { cache: useCache, parallel } = options;
+    this.cacheDir = useCache === true ? findCacheDir({ name: 'uglifyjs-webpack-plugin' }) : useCache;


this.cacheDir = cache === true ? findCacheDir({ name: 'uglifyjs-webpack-plugin' }) : cache; this.cache = new Cache(this.cacheDir);

michael-ciniawsky · 2017-10-22T23:21:01Z

src/uglify/index.js

@@ -58,15 +58,21 @@ export default class {
          const done = () => step(index, result);

          if (this.cacheDir && !result.error) {
-            cacache.put(this.cacheDir, task.cacheKey, JSON.stringify(data)).then(done, done);
+            cache.put(this.cacheDir, task.cacheKey, JSON.stringify(data));


this.cache.put(task.cacheKey, JSON.stringify(data))

filipesilva · 2017-10-23T07:35:39Z

@michael-ciniawsky the approach that you're suggesting (using a instanced class) will not work with the tests as they exist currently, since they rely on being able to spy on static cache methods. It would also require the cache being exposed cache instance being exposed so the tests could tap into it. Is this something you want changed?

Blocked by #108. See #120 for reasoning. Fix #120.

hansl · 2017-10-23T15:20:56Z

@michael-ciniawsky the process of relicensing cacache would likely take weeks, and we've (Angular CLI's team) been waiting since July for this release to have a proper license. The current adaptor would allow us to change back to cacache when they do support an open source license, without any changes to the code.

joshwiens · 2017-10-23T15:33:21Z

FYI - In it's current state this throws when trying to access cached build assets.

filipesilva · 2017-10-23T15:36:13Z

@d3viant0ne did you pull the new changes? I tried a rebuild some 10m ago and rebuilds were good. I amended the previous commit with the fix some 20m ago.

zkat · 2017-10-23T18:03:09Z

Heads-up: I'm working on relicensing cacache, if it makes any difference here. Should be ready soonish, since folks are 👍 pretty quick and there aren't a lot of contributors rn.

Edit: I took a look at flat-cache and I suggest y'all find something different. I don't think that library's gonna hold up at all under concurrency conditions. The most likely scenario is you'll have an eternally-invalidated cache, since you're going to have a race that looks like:

worker 1                               worker 2
 |                                      | - truncate cache file
 |                                      | - write chunk 1
 | - truncate cache file                |
 |                                      | - write chunk 2
 | - write chunk 1                      |
 |                                      | -  close cache file                          
 |
 | - write chunk 2
 | _ close cache file

meaning:

"foo": 1
"bar": 2
} // end of worker 2, chunk 2
"a": 1, // worker 1, chunk 1
"b": 2,
"c": 3
} // end of worker 1, chunk 1

And as soon as flat-cache reloads that file, it's just going to throw it all away

zkat · 2017-10-23T18:21:15Z

@hansl fwiw, no one told me about this until a couple of days ago, and I needed an excuse to start the relicense process, since it needed to happen anyway.

michael-ciniawsky · 2017-10-23T20:00:26Z

Fixed by #145

filipesilva · 2017-10-23T22:20:26Z

Heya @zkat, I just wanted to thank you for being so speedy in relicensing cacache! That was really fast!

You're analysis of flat-cache under concurrency conditions also makes a lot of sense. That hadn't occurred to me at all :/

michael-ciniawsky · 2017-10-23T22:23:44Z

Yep, thanks again @zkat for all the (quick) help here 👍

filipesilva changed the title ~~refactor: replace cacache with flat-cache~~ [WIP] refactor: replace cacache with flat-cache Sep 6, 2017

filipesilva mentioned this pull request Sep 6, 2017

[Refactor] swap cacache with another cache #120

Closed

alexander-akait self-assigned this Sep 6, 2017

alexander-akait added priority: 4 (important) pr_action: Blocked semver: Patch type: Feature labels Sep 6, 2017

blowery mentioned this pull request Oct 6, 2017

Build: Remove custom bundler and extract minification Automattic/wp-calypso#18074

Merged

joshwiens added priority: 2 (urgent) and removed priority: 4 (important) labels Oct 20, 2017

michael-ciniawsky added pr_action: Rework pr_action: Rebase and removed pr_action: Blocked pr_action: Rework labels Oct 21, 2017

michael-ciniawsky added type: Refactor and removed type: Feature labels Oct 21, 2017

michael-ciniawsky added this to the 1.0.0 milestone Oct 21, 2017

joshwiens assigned joshwiens and unassigned alexander-akait Oct 21, 2017

filipesilva changed the title ~~[WIP] refactor: replace cacache with flat-cache~~ refactor: replace cacache with flat-cache Oct 22, 2017

michael-ciniawsky suggested changes Oct 22, 2017

View reviewed changes

michael-ciniawsky added pr_action: Review and removed pr_action: Rebase labels Oct 22, 2017

michael-ciniawsky reviewed Oct 22, 2017

View reviewed changes

refactor: replace cacache with flat-cache

1f7db40

Blocked by #108. See #120 for reasoning. Fix #120.

zkat mentioned this pull request Oct 23, 2017

fix(deps): cacache@10 with ISC licence #145

Merged

michael-ciniawsky closed this Oct 23, 2017

michael-ciniawsky added status: Declined and removed priority: 2 (urgent) pr_action: Review labels Oct 23, 2017

michael-ciniawsky removed this from the 1.0.0 milestone Oct 23, 2017

filipesilva deleted the replace-cacache branch October 23, 2017 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace cacache with flat-cache #121

refactor: replace cacache with flat-cache #121

filipesilva commented Sep 6, 2017 •

edited by michael-ciniawsky

Loading

joshwiens commented Oct 20, 2017 •

edited

Loading

filipesilva commented Oct 20, 2017

michael-ciniawsky commented Oct 20, 2017

joshwiens commented Oct 20, 2017 •

edited

Loading

michael-ciniawsky commented Oct 21, 2017

michael-ciniawsky commented Oct 21, 2017

alexander-akait commented Oct 21, 2017

filipesilva commented Oct 22, 2017

filipesilva commented Oct 22, 2017

filipesilva commented Oct 22, 2017

michael-ciniawsky left a comment

michael-ciniawsky Oct 22, 2017

filipesilva Oct 22, 2017

filipesilva Oct 22, 2017

michael-ciniawsky Oct 22, 2017

michael-ciniawsky Oct 22, 2017

filipesilva Oct 22, 2017

michael-ciniawsky Oct 22, 2017

filipesilva Oct 22, 2017

michael-ciniawsky Oct 22, 2017

michael-ciniawsky Oct 22, 2017 •

edited

Loading

michael-ciniawsky Oct 22, 2017

michael-ciniawsky Oct 22, 2017

michael-ciniawsky Oct 22, 2017

filipesilva commented Oct 23, 2017

hansl commented Oct 23, 2017

joshwiens commented Oct 23, 2017

filipesilva commented Oct 23, 2017

zkat commented Oct 23, 2017 •

edited

Loading

zkat commented Oct 23, 2017

michael-ciniawsky commented Oct 23, 2017

filipesilva commented Oct 23, 2017

michael-ciniawsky commented Oct 23, 2017


		const cacheId = 'uglifyjs-webpack-plugin';

		export default class Cache {

		@@ -0,0 +1,23 @@
		import flatCache from 'flat-cache';

		const cacheId = 'uglifyjs-webpack-plugin';

refactor: replace cacache with flat-cache #121

refactor: replace cacache with flat-cache #121

Conversation

filipesilva commented Sep 6, 2017 • edited by michael-ciniawsky Loading

Issues

joshwiens commented Oct 20, 2017 • edited Loading

filipesilva commented Oct 20, 2017

michael-ciniawsky commented Oct 20, 2017

joshwiens commented Oct 20, 2017 • edited Loading

michael-ciniawsky commented Oct 21, 2017

michael-ciniawsky commented Oct 21, 2017

alexander-akait commented Oct 21, 2017

filipesilva commented Oct 22, 2017

filipesilva commented Oct 22, 2017

filipesilva commented Oct 22, 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

michael-ciniawsky Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Alternative

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipesilva commented Oct 23, 2017

hansl commented Oct 23, 2017

joshwiens commented Oct 23, 2017

filipesilva commented Oct 23, 2017

zkat commented Oct 23, 2017 • edited Loading

zkat commented Oct 23, 2017

michael-ciniawsky commented Oct 23, 2017

filipesilva commented Oct 23, 2017

michael-ciniawsky commented Oct 23, 2017

filipesilva commented Sep 6, 2017 •

edited by michael-ciniawsky

Loading

`Issues`

joshwiens commented Oct 20, 2017 •

edited

Loading

joshwiens commented Oct 20, 2017 •

edited

Loading

michael-ciniawsky Oct 22, 2017 •

edited

Loading

`Alternative`

zkat commented Oct 23, 2017 •

edited

Loading