Generate cache keys for sources and collections #179
Conversation
mikrostew
changed the title
lib/assets/AssetsSource.js
Outdated
| * @returns {String} the cache key | ||
| */ | ||
| AssetsSource.prototype.cacheKey = function(namespace) { | ||
| function skipSomeKeys(key, value) { |
There was a problem hiding this comment.
I don't believe this closes over anything useful, so lets hoist this out to the root of the module, this will prevent the necessary redefinition of skipSomeKeys each time cacheKey function is invoked
test/test_assets.js
Outdated
| }); | ||
| assert.equal(source1.cacheKey(), source2.cacheKey()); | ||
| assert.notEqual(source1.cacheKey(), source3.cacheKey()); | ||
| done(); |
There was a problem hiding this comment.
In mocha, we should likely not use done(), two reasons:
- sync tests, such as this don't require it. It can safely be dropped.
- async tests, should preferrably rely on a promise being returned in the
itfunctionbody, as it does error handling correctly (timed out tests do to test failures suck)
There was a problem hiding this comment.
ok, I'll remove the use of done() in all these places
test/test_assets.js
Outdated
| }); | ||
| assert.equal(source1.cacheKey(), source2.cacheKey()); | ||
| assert.notEqual(source1.cacheKey(), source3.cacheKey()); | ||
| done(); |
test/test_assets.js
Outdated
| var source2 = new AssetsSource(rootDir, {}); | ||
| assert.equal(source1.cacheKey("foo"), source2.cacheKey("foo")); | ||
| assert.notEqual(source1.cacheKey("foo"), source2.cacheKey("bar")); | ||
| done(); |
test/test_assets.js
Outdated
| var source3 = new AssetsSource(rootDir2, {}); | ||
| assert.equal(source1.cacheKey(), source2.cacheKey()); | ||
| assert.notEqual(source1.cacheKey(), source3.cacheKey()); | ||
| done(); |
test/test_assets.js
Outdated
| }); | ||
| assert.equal(source1.cacheKey(), source2.cacheKey()); | ||
| assert.equal(source1.cacheKey(), source3.cacheKey()); | ||
| done(); |
test/test_assets.js
Outdated
| assert.equal(collection1.cacheKey(), collection2.cacheKey()); | ||
| assert.notEqual(collection1.cacheKey(), collection3.cacheKey()); | ||
| assert.notEqual(collection1.cacheKey(), collection4.cacheKey()); | ||
| done(); |
lib/assets/AssetsSource.js
Outdated
| ";name=" + (this.name ? this.name : (namespace ? namespace : "")) + | ||
| ";srcPath=" + this.srcPath + | ||
| ";pattern=" + this.pattern + | ||
| ";opts=" + JSON.stringify(this.globOpts, skipSomeKeys) + |
There was a problem hiding this comment.
JSON.stringify isn't stable, so it will sometimes produce different ordering of keys in the output. I would recommend using json-stable-stringify instead.
There was a problem hiding this comment.
oh ok, I didn't know that - I'll change that
There was a problem hiding this comment.
ya, i found out the hard way...
| @@ -57,6 +59,7 @@ Assets.prototype.addSource = function(src, opts) { | |||
| */ | |||
| Assets.prototype.export = function(src, opts) { | |||
| var assets = new AssetsCollection(); | |||
| this.moduleCollections.push(assets); | |||
There was a problem hiding this comment.
Where is this used? If it is new public API, we should be sure to add appropriate tests so that it doesn't vanish.
There was a problem hiding this comment.
yes, this is new API to have access to the AssetsCollections in modules (for caching them)
lib/assets/AssetsCollection.js
Outdated
| * @returns {String} the cache key | ||
| */ | ||
| AssetsCollection.prototype.cacheKey = function(name) { | ||
| return this.sources.reduce(function(cacheStr, source) { |
There was a problem hiding this comment.
should we sort sources to ensure they remain stable for cacheKey generation?
|
released as v1.4.0 🎉 |
Adds a few things to allow build tools to cache the Scss generated in asAssetImport(), since otherwise this is done over and over: