switch to nested immutables state

src/engine/blocks.js

@@ -1,6 +1,7 @@
 const adapter = require('./adapter');
 const mutationAdapter = require('./mutation-adapter');
 const xmlEscape = require('../util/xml-escape');
+const {Map} = require('immutable');
 
 /**
  * @fileoverview
@@ -293,22 +294,19 @@ class Blocks {
             if (optRuntime && wasMonitored && !block.isMonitored) {
                 optRuntime.requestRemoveMonitor(block.id);
             } else if (optRuntime && !wasMonitored && block.isMonitored) {
-                optRuntime.requestAddMonitor(
+                optRuntime.requestAddMonitor(Map({
-                    // Ensure that value is not undefined, since React requires it
-                    {
                     // @todo(vm#564) this will collide if multiple sprites use same block
                     id: block.id,
                     category: 'data',
                     // @todo(vm#565) how to handle translation here?
                     label: block.opcode,
                     // @todo(vm#565) for numerical values with decimals, some countries use comma
-                        value: '',
+                    value: '', // Ensure that value is not undefined, since React requires it
                     x: 0,
                     // @todo(vm#566) Don't require sending x and y when instantiating a
                     // monitor. If it's not preset the GUI should decide.
                     y: 0
-                    }
+                }));
-                );
             }
             break;
         }

src/engine/execute.js

@@ -1,5 +1,6 @@
 const log = require('../util/log');
 const Thread = require('./thread');
+const {Map} = require('immutable');
 
 /**
  * Utility function to determine if a value is a Promise.
@@ -97,10 +98,10 @@ const execute = function (sequencer, thread) {
                     runtime.visualReport(currentBlockId, resolvedValue);
                 }
                 if (thread.updateMonitor) {
-                    runtime.requestUpdateMonitor({
+                    runtime.requestUpdateMonitor(Map({
                         id: currentBlockId,
                         value: String(resolvedValue)
-                    });
+                    }));
                 }
             }
             // Finished any yields.

src/engine/runtime.js

@@ -706,7 +706,8 @@ class Runtime extends EventEmitter {
         }
 
         if (!this._prevMonitorState.equals(this._monitorState)) {
-            this.emit(Runtime.MONITORS_UPDATE, this._monitorState.toArray());
+            const monitorStateObj = this._monitorState.toJS();
+            this.emit(Runtime.MONITORS_UPDATE, Object.keys(monitorStateObj).map(key => monitorStateObj[key]));
         }
         this._prevMonitorState = this._monitorState;
     }
@@ -880,51 +881,28 @@ class Runtime extends EventEmitter {
     /**
      * Add a monitor to the state. If the monitor already exists in the state,
      * overwrites it.
-     * @param {!object} monitor Monitor to add.
+     * @param {!Map} monitor Monitor to add.
      */
     requestAddMonitor (monitor) {
-        this._monitorState = this._monitorState.set(monitor.id, monitor);
+        this._monitorState = this._monitorState.set(monitor.get('id'), monitor);
     }
 
     /**
      * Update a monitor in the state. Does nothing if the monitor does not already
      * exist in the state.
-     * @param {!object} monitor Monitor to update.
+     * @param {!Map} monitor Monitor to update.
      */
     requestUpdateMonitor (monitor) {
-        if (this._monitorState.has(monitor.id) &&
+        if (this._monitorState.has(monitor.get('id'))) {
-                this._monitorWouldChange(this._monitorState.get(monitor.id), monitor)) {
             this._monitorState =
-                this._monitorState.set(monitor.id, Object.assign({}, this._monitorState.get(monitor.id), monitor));
+                this._monitorState.set(monitor.get('id'), this._monitorState.get(monitor.get('id')).merge(monitor));
         }
     }
 
-    /**
-     * Whether or not the monitor would change when the new state is applied. New state has its properties
-     * applied to the current state, it does not completely overwrite the current state. For this to return
-     * accurately, the changes being applied should be on primitive types, otherwise it may false-positive.
-     * @param {!object} currentMonitorState the state of a monitor, the values in this._monitorState
-     * @param {!object} newMonitorDelta the state to be applied to a monitor. The IDs of current and new monitor
-     * should match.
-     * @return {boolean} whether the new state, when applied to the current state, would change the current state.
-     */
-    _monitorWouldChange (currentMonitorState, newMonitorDelta) {
-        for (const prop in newMonitorDelta) {
-            if (currentMonitorState.hasOwnProperty(prop)) {
-                // Strict equals to check if properties change; switch this to deep equals if
-                // we expect monitors to have changing state that is not primative.
-                if (currentMonitorState[prop] !== newMonitorDelta[prop]) return true;
-            } else {
-                return true; // The new state would add a new property to the monitor
-            }
-        }
-        return false;
-    }
-
     /**
      * Removes a monitor from the state. Does nothing if the monitor already does
      * not exist in the state.
-     * @param {!object} monitorId ID of the monitor to remove.
+     * @param {!string} monitorId ID of the monitor to remove.
      */
     requestRemoveMonitor (monitorId) {
         this._monitorState = this._monitorState.delete(monitorId);

test/unit/engine_runtime.js

@@ -1,5 +1,6 @@
 const test = require('tap').test;
 const Runtime = require('../../src/engine/runtime');
+const {Map} = require('immutable');
 
 test('spec', t => {
     const r = new Runtime();
@@ -11,48 +12,62 @@ test('spec', t => {
     t.end();
 });
 
-test('monitorWouldChange_false', t => {
+test('monitorStateEquals', t => {
     const r = new Runtime();
-    const currentMonitorState = {
+    const id = 'xklj4#!';
-        id: 'xklj4#!',
+    const prevMonitorState = Map({
+        id,
         category: 'data',
         label: 'turtle whereabouts',
         value: '25',
         x: 0,
         y: 0
-    };
+    });
-    const newMonitorDelta = {
+    const newMonitorDelta = Map({
-        id: 'xklj4#!',
+        id,
         value: String(25)
-    };
+    });
-    t.equals(false, r._monitorWouldChange(currentMonitorState, newMonitorDelta));
+    r.requestAddMonitor(prevMonitorState);
+    r.requestUpdateMonitor(newMonitorDelta);
+
+    t.equals(true, prevMonitorState.equals(r._monitorState.get(id)));
+    t.equals(String(25), r._monitorState.get(id).get('value'));
     t.end();
 });
 
-test('monitorWouldChange_true', t => {
+test('monitorStateDoesNotEqual', t => {
     const r = new Runtime();
-    const currentMonitorState = {
+    const id = 'xklj4#!';
-        id: 'xklj4#!',
+    const prevMonitorState = Map({
+        id,
         category: 'data',
         label: 'turtle whereabouts',
         value: '25',
         x: 0,
         y: 0
-    };
+    });
 
     // Value change
-    let newMonitorDelta = {
+    let newMonitorDelta = Map({
-        id: 'xklj4#!',
+        id,
         value: String(24)
-    };
+    });
-    t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta));
+    r.requestAddMonitor(prevMonitorState);
+    r.requestUpdateMonitor(newMonitorDelta);
+    
+    t.equals(false, prevMonitorState.equals(r._monitorState.get(id)));
+    t.equals(String(24), r._monitorState.get(id).get('value'));
 
     // Prop change
-    newMonitorDelta = {
+    newMonitorDelta = Map({
         id: 'xklj4#!',
         moose: 7
-    };
+    });
-    t.equal(true, r._monitorWouldChange(currentMonitorState, newMonitorDelta));
+    r.requestUpdateMonitor(newMonitorDelta);
+
+    t.equals(false, prevMonitorState.equals(r._monitorState.get(id)));
+    t.equals(String(24), r._monitorState.get(id).get('value'));
+    t.equals(7, r._monitorState.get(id).get('moose'));
 
     t.end();
 });