Make sure saving file doesn't overwrite config

Fix yeoman/yo#275
yeoman · Jan 22, 2015 · 13142a3 · 13142a3
1 parent b611741
commit 13142a3
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 50 deletions.
diff --git a/lib/util/storage.js b/lib/util/storage.js
@@ -41,16 +41,24 @@ Storage.prototype._store = function () {
   return this.fs.readJSON(this.path, {})[this.name] || {};
 };
 
+/**
+ * Persist a configuration to disk
+ * @param {Object} val - current configuration values
+ */
+Storage.prototype._persist = function (val) {
+  var fullStore = this.fs.readJSON(this.path, {});
+  fullStore[this.name] = val;
+  this.fs.write(this.path, JSON.stringify(fullStore, null, '  '));
+};
+
 /**
  * Save a new object of values
  * @param {Object} val - Store new state
  * @return {null}
  */
 
-Storage.prototype.save = function (val) {
-  var fullStore = this.fs.readJSON(this.path, {});
-  fullStore[this.name] = val;
-  this.fs.write(this.path, JSON.stringify(fullStore, null, '  '));
+Storage.prototype.save = function () {
+  this._persist(this._store());
 };
 
 /**
@@ -59,7 +67,7 @@ Storage.prototype.save = function (val) {
  * @return {null}
  */
 
-Storage.prototype.forceSave = function (val) { this.save(val); };
+Storage.prototype.forceSave = function () { this.save(); };
 
 /**
  * Get a stored value
@@ -97,7 +105,7 @@ Storage.prototype.set = function (key, val) {
   } else {
     store[key] = val;
   }
-  this.save(store);
+  this._persist(store);
   return val;
 };
 
@@ -110,7 +118,7 @@ Storage.prototype.set = function (key, val) {
 Storage.prototype.delete = function (key) {
   var store = this._store();
   delete store[key];
-  this.save(store);
+  this._persist(store);
 };
 
 /**

diff --git a/test/storage.js b/test/storage.js
@@ -28,7 +28,6 @@ describe('Storage', function () {
     this.fs = FileEditor.create(this.memFs);
     this.store = new Storage('test', this.fs, this.storePath);
     this.store.set('foo', 'bar');
-    this.saveSpy = sinon.spy(this.store, 'save');
   });
 
   afterEach(function () {
@@ -90,9 +89,9 @@ describe('Storage', function () {
 
     it('save on each changes', function () {
       this.store.set('foo', 'bar');
-      assert.equal(this.saveSpy.callCount, 1);
+      assert.equal(this.fs.readJSON(this.storePath).test.foo, 'bar');
       this.store.set('foo', 'oo');
-      assert.equal(this.saveSpy.callCount, 2);
+      assert.equal(this.fs.readJSON(this.storePath).test.foo, 'oo');
     });
 
     describe('@return', function () {
@@ -124,74 +123,68 @@ describe('Storage', function () {
         );
       });
     });
-  });
-
-  describe('#getAll()', function () {
-    beforeEach(function () {
-      this.store.set({ foo: 'bar', john: 'doe' });
-    });
-
-    it('get all values', function () {
-      assert.deepEqual(this.store.getAll().foo, 'bar');
-    });
-
-    it('does not return a reference to the inner store', function () {
-      this.store.getAll().foo = 'uhoh';
-      assert.equal(this.store.getAll().foo, 'bar');
-    });
-  });
-
-  describe('#delete()', function () {
-    beforeEach(function () {
-      this.store.set('name', 'test');
-    });
-
-    it('delete value', function () {
-      this.store.delete('name');
-      assert.equal(this.store.get('name'), undefined);
-    });
-  });
-
-  describe('#save()', function () {
-    beforeEach(function () {
-      this.saveStorePath = path.join(tmpdir, 'save.json');
-      rm(this.saveStorePath);
-      this.store = new Storage('test', this.fs, this.saveStorePath);
-      this.store.set('foo', 'bar');
-      this.saveSpy = sinon.spy(this.store, 'save');
-    });
 
     describe('when multiples instances share the same file', function () {
       beforeEach(function () {
-        this.store2 = new Storage('test2', this.fs, this.saveStorePath);
+        this.store = new Storage('test', this.fs, this.storePath);
+        this.store.set('foo', 'bar');
+        this.store2 = new Storage('test2', this.fs, this.storePath);
       });
 
       it('only update modified namespace', function () {
         this.store2.set('bar', 'foo');
         this.store.set('foo', 'bar');
 
-        var json = this.fs.readJSON(this.saveStorePath);
+        var json = this.fs.readJSON(this.storePath);
         assert.equal(json.test.foo, 'bar');
         assert.equal(json.test2.bar, 'foo');
       });
     });
 
     describe('when multiples instances share the same namespace', function () {
       beforeEach(function () {
-        this.store2 = new Storage('test', this.fs, this.saveStorePath);
+        this.store = new Storage('test', this.fs, this.storePath);
+        this.store.set('foo', 'bar');
+        this.store2 = new Storage('test', this.fs, this.storePath);
       });
 
       it('only update modified namespace', function () {
         this.store2.set('bar', 'foo');
         this.store.set('foo', 'bar');
 
-        var json = this.fs.readJSON(this.saveStorePath);
+        var json = this.fs.readJSON(this.storePath);
         assert.equal(json.test.foo, 'bar');
         assert.equal(json.test.bar, 'foo');
       });
     });
   });
 
+  describe('#getAll()', function () {
+    beforeEach(function () {
+      this.store.set({ foo: 'bar', john: 'doe' });
+    });
+
+    it('get all values', function () {
+      assert.deepEqual(this.store.getAll().foo, 'bar');
+    });
+
+    it('does not return a reference to the inner store', function () {
+      this.store.getAll().foo = 'uhoh';
+      assert.equal(this.store.getAll().foo, 'bar');
+    });
+  });
+
+  describe('#delete()', function () {
+    beforeEach(function () {
+      this.store.set('name', 'test');
+    });
+
+    it('delete value', function () {
+      this.store.delete('name');
+      assert.equal(this.store.get('name'), undefined);
+    });
+  });
+
   describe('#defaults()', function () {
     beforeEach(function () {
       this.store.set('val1', 1);