test: storage integration tests and fixes

src/storage.js

@@ -57,23 +57,37 @@ function setupGlobalStorageBrowser() {
         CHANGE_TYPE_INTERNAL = "Internal";
     const MGS_CHANGE = 'change';
     const cache = {};
-    let pendingBroadcast = {},
+    let pendingBroadcastKV = {}, // map from watched keys that was set in this instance to
+        // modified time and value - key->{t,v}
         watchExternalKeys = {},
         externalWatchKeyList = [];
 
     const storageChannel = new BroadcastChannel(PHOENIX_STORAGE_BROADCAST_CHANNEL_NAME);
-    function _broadcastPendingChanges() {
-        storageChannel.postMessage({type: MGS_CHANGE, keys: Object.keys(pendingBroadcast)});
-        pendingBroadcast = {};
-    }
-    setInterval(_broadcastPendingChanges, EXTERNAL_CHANGE_BROADCAST_INTERVAL);
+    setInterval(()=>{
+        // broadcast all changes made to watched keys in this instance to others
+        storageChannel.postMessage({type: MGS_CHANGE, keys: pendingBroadcastKV});
+        pendingBroadcastKV = {};
+
+    }, EXTERNAL_CHANGE_BROADCAST_INTERVAL);
     // Listen for messages on the channel
     storageChannel.onmessage = (event) => {
         const message = event.data;
         if(message.type === MGS_CHANGE){
-            for(let key of message.keys){
-                PhStore.trigger(key, CHANGE_TYPE_EXTERNAL);
-                delete cache[key]; // clear cache for changed data
+            const changedKV = message.keys;
+            for(let key of Object.keys(changedKV)){
+                // we only update the key and trigger if the key is being watched here.
+                // If unwatched keys are updated from another window, for eg, theme change pulled in from a new
+                // theme installed in another window cannot be applied in this window. So the code has to
+                // explicitly support external changes by calling watchExternalChanges API.
+                if(watchExternalKeys[key]) {
+                    const externalChange = changedKV[key]; // {t,v} in new value, t = changed time
+                    // if the change time of the external event we got is more recent than what we have,
+                    // only then accept the change. else we have more recent data.
+                    if(!cache[key] || (externalChange.t > cache[key].t)) {
+                        cache[key] = externalChange;
+                        PhStore.trigger(key, CHANGE_TYPE_EXTERNAL);
+                    }
+                }
             }
         }
     };
@@ -93,7 +107,7 @@ function setupGlobalStorageBrowser() {
     function getItem(key) {
         let cachedResult = cache[key];
         if(cachedResult){
-            return cachedResult;
+            return JSON.parse(cachedResult.v);
         }
         const jsonStr = localStorage.getItem(PH_LOCAL_STORE_PREFIX + key);
         if(jsonStr === null){
@@ -102,7 +116,7 @@ function setupGlobalStorageBrowser() {
         try {
             cachedResult = JSON.parse(jsonStr);
             cache[key] = cachedResult;
-            return cachedResult;
+            return JSON.parse(cachedResult.v); // clone. JSON.Parse is faster than structured object clone.
         } catch (e) {
             return null;
         }
@@ -116,10 +130,16 @@ function setupGlobalStorageBrowser() {
      *
      */
     function setItem(key, value) {
-        localStorage.setItem(PH_LOCAL_STORE_PREFIX + key, JSON.stringify(value));
-        cache[key] = value;
+        const valueToStore = {
+            t: Date.now(), // modified time
+            // we store value as string here as during get operation, we can use json.parse to clone instead of
+            // using slower structured object clone.
+            v: JSON.stringify(value)
+        };
+        localStorage.setItem(PH_LOCAL_STORE_PREFIX + key, JSON.stringify(valueToStore));
+        cache[key] = valueToStore;
         if(watchExternalKeys[key]){
-            pendingBroadcast[key] = true;
+            pendingBroadcastKV[key] = valueToStore;
         }
         PhStore.trigger(key, CHANGE_TYPE_INTERNAL);
     }

test/spec/Storage-integ-test.js

@@ -19,7 +19,7 @@
  *
  */
 
-/*global PhStore, describe, it, expect, beforeAll, afterAll, awaitsForDone */
+/*global PhStore, describe, it, expect, beforeAll, afterAll, awaits, awaitsFor, afterEach */
 
 define(function (require, exports, module) {
     // Recommended to avoid reloading the integration test window Phoenix instance for each test.
@@ -32,6 +32,7 @@ define(function (require, exports, module) {
 
 
     describe("integration:Storage integration tests", function () {
+        const testKey = "test_storage_key";
 
         beforeAll(async function () {
             // do not use force option in brackets core integration tests. Tests are assumed to reuse the existing
@@ -44,8 +45,90 @@ define(function (require, exports, module) {
             await SpecRunnerUtils.closeTestWindow();
         }, 30000);
 
-        it("Should PhStore APIs be available", async function () { // #2813
+        afterEach(async function () {
+            PhStore.unwatchExternalChanges(testKey);
+            PhStore.off(testKey);
+            testWindow.PhStore.unwatchExternalChanges(testKey);
+        });
+
+        it("Should PhStore APIs be available", async function () {
             expect(PhStore).toBeDefined();
+            expect(PhStore.setItem).toBeDefined();
+            expect(PhStore.getItem).toBeDefined();
+            expect(PhStore.watchExternalChanges).toBeDefined();
+            expect(PhStore.unwatchExternalChanges).toBeDefined();
+        });
+
+        function expectSetGetSuccess(value) {
+            PhStore.setItem(testKey, value);
+            expect(PhStore.getItem(testKey)).toEql(value);
+        }
+
+        it("Should be able to get and set different value types", async function () {
+            expectSetGetSuccess(1);
+            expectSetGetSuccess("");
+            expectSetGetSuccess(0);
+            expectSetGetSuccess("hello");
+            expectSetGetSuccess({hello: {message: "world"}});
+            expectSetGetSuccess([1, "3"]);
+        });
+
+        it("Should mutating the item got from get API not change the actual object for next get", async function () {
+            PhStore.setItem(testKey, {hello: "world"});
+            const item = PhStore.getItem(testKey);
+            item.hello = "new World";
+            const itemAgain = PhStore.getItem(testKey);
+            // each get should get a clone of the object, so mutations on previous get shouldn't affect the getItem call
+            expect(itemAgain.hello).toEqual("world");
+        });
+
+        it("Should return cached content if we are not watching for external changes", async function () {
+            const internal = "internal", external = "external";
+            PhStore.setItem(testKey, internal);
+            expect(PhStore.getItem(testKey)).toEql(internal); // this will cache value locally
+
+            testWindow.PhStore.watchExternalChanges(testKey); // watch in phcode window, should not affect cache in this window
+            testWindow.PhStore.setItem(testKey, external);
+            await awaits(1000);
+            expect(PhStore.getItem(testKey)).toEql(internal);
+            expect(testWindow.PhStore.getItem(testKey)).toEql(external);
+        });
+
+        it("Should get changed notification in this window, not watching external changes", async function () {
+            PhStore.setItem(testKey, 1);
+            let changeType;
+            PhStore.on(testKey, (_event, type)=>{
+                changeType = type;
+            });
+            const newValue = "hello";
+            PhStore.setItem(testKey, newValue);
+            expect(PhStore.getItem(testKey)).toEql(newValue);
+            expect(changeType).toEql("Internal"); // this event raising is synchronous
+        });
+
+        it("Should get changed notification in this window, if watching for external changes", async function () {
+            const currentWinVal = "currentwindow";
+            PhStore.setItem(testKey, currentWinVal);
+            expect(PhStore.getItem(testKey)).toEql(currentWinVal);
+
+            PhStore.watchExternalChanges(testKey);
+            testWindow.PhStore.watchExternalChanges(testKey);
+
+            let changeType, changedValue;
+            PhStore.on(testKey, (_event, type)=>{
+                changeType = type;
+                changedValue = PhStore.getItem(testKey); // the new key should be updated when you get the event
+            });
+
+            const newValue = "hello";
+            await awaits(3);// let 3ms time pass as the lowest resolution for time check in browser is 1 ms
+            testWindow.PhStore.setItem(testKey, newValue); // set in phoenix, it should eventually come to this window
+            expect(testWindow.PhStore.getItem(testKey)).toEql(newValue);
+            await awaitsFor(function () {
+                return changedValue === newValue;
+            });
+            expect(changeType).toEql("External");
+            expect(changedValue).toEql(newValue);
         });
 
     });

test/spec/Template-for-integ-test.js

@@ -62,7 +62,7 @@ define(function (require, exports, module) {
             await SpecRunnerUtils.closeTestWindow();
         }, 30000);
 
-        it("Should open file in project", async function () { // #2813
+        it("Should open file in project", async function () {
             await awaitsForDone(
                 FileViewController.openAndSelectDocument(
                     testPath + "/simple.js",
@@ -74,7 +74,7 @@ define(function (require, exports, module) {
                 "closing all file");
         });
 
-        it("Should open file in project and add to working set", async function () { // #2813
+        it("Should open file in project and add to working set", async function () {
             await awaitsForDone(FileViewController.openFileAndAddToWorkingSet(testPath + "/edit.js"));
             const selected = MainViewManager.findInAllWorkingSets(testPath + "/edit.js");
             expect(selected.length >= 1 ).toBe(true);