Debug UI: fixes related to errors in arguments (#123)

* debug: fix argument evalution involving commas * debug: implement DebugMove error ui * remove unnecessary prop types
boardgameio · Feb 27, 2018 · 6a010c8 · 6a010c8
1 parent 1e5dd83
commit 6a010c8
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 62 deletions.
diff --git a/src/client/debug/debug.css b/src/client/debug/debug.css
@@ -47,6 +47,11 @@
   font-weight: bold;
 }
 
+.debug-ui .move-error {
+  color: #a00;
+  font-weight: bold;
+}
+
 .debug-ui .arg-field {
   outline: none;
   font-family: monospace;

diff --git a/src/client/debug/debug.js b/src/client/debug/debug.js
@@ -24,58 +24,69 @@ import './debug.css';
 export class DebugMove extends React.Component {
   static propTypes = {
     name: PropTypes.string.isRequired,
+    shortcut: PropTypes.string.isRequired,
     fn: PropTypes.func.isRequired,
-    active: PropTypes.bool,
-    activate: PropTypes.func,
-    deactivate: PropTypes.func,
   };
 
   state = {
     error: '',
   };
 
-  onClick = () => {
-    this.props.activate();
-  };
+  onSubmit = value => {
+    let error = '';
 
-  getArg(arg) {
     try {
-      return new Function('return ' + arg)();
+      let argArray = new Function(`return [${value}]`)();
+      this.props.fn.apply(this, argArray);
     } catch (e) {
-      return undefined;
+      error = '' + e;
     }
-  }
-
-  onSubmit = () => {
-    let argArray = [];
-    const value = this.span.innerText;
-
-    if (value && value.length) {
-      const args = value.split(',');
-      for (const arg of args) {
-        argArray.push(this.getArg(arg));
-      }
-    }
-
-    this.props.fn.apply(this, argArray);
 
     this.setState({
-      error: '',
+      error,
       focus: false,
       enterArg: false,
     });
+  };
 
-    this.span.innerText = '';
+  render() {
+    return (
+      <div>
+        <KeyboardShortcut value={this.props.shortcut}>
+          <DebugMoveArgField name={this.props.name} onSubmit={this.onSubmit} />
+        </KeyboardShortcut>
+        {this.state.error ? (
+          <span className="move-error">{this.state.error}</span>
+        ) : null}
+      </div>
+    );
+  }
+}
 
-    if (this.props.deactivate) {
-      this.props.deactivate();
-    }
+export class DebugMoveArgField extends React.Component {
+  static propTypes = {
+    name: PropTypes.string.isRequired,
+    onSubmit: PropTypes.func.isRequired,
+    active: PropTypes.bool,
+    activate: PropTypes.func,
+    deactivate: PropTypes.func,
   };
 
+  componentDidUpdate() {
+    if (this.props.active) {
+      this.span.focus();
+    } else {
+      this.span.blur();
+    }
+  }
+
   onKeyDown = e => {
     if (e.key == 'Enter') {
       e.preventDefault();
-      this.onSubmit();
+      const value = this.span.innerText;
+      this.props.onSubmit(value);
+      this.span.innerText = '';
+      this.props.deactivate();
     }
 
     if (e.key == 'Escape') {
@@ -84,26 +95,18 @@ export class DebugMove extends React.Component {
     }
   };
 
-  componentDidUpdate() {
-    if (this.props.active) {
-      this.span.focus();
-    } else {
-      this.span.blur();
-    }
-  }
-
   render() {
     let className = 'move';
     if (this.props.active) className += ' active';
     return (
-      <div className={className} onClick={this.onClick}>
+      <div className={className} onClick={this.props.activate}>
         {this.props.name}
         (<span
           ref={r => {
             this.span = r;
           }}
           className="arg-field"
-          onBlur={() => this.props.deactivate()}
+          onBlur={this.props.deactivate}
           onKeyDown={this.onKeyDown}
           contentEditable
         />)
@@ -354,9 +357,7 @@ export class Debug extends React.Component {
       const fn = this.props.moves[name];
       const shortcut = this.shortcuts[name];
       moves.push(
-        <KeyboardShortcut key={name} value={shortcut}>
-          <DebugMove name={name} fn={fn} />
-        </KeyboardShortcut>
+        <DebugMove key={name} name={name} fn={fn} shortcut={shortcut} />
       );
     }
 
@@ -365,9 +366,7 @@ export class Debug extends React.Component {
       const fn = this.props.events[name];
       const shortcut = this.shortcuts[name];
       events.push(
-        <KeyboardShortcut key={name} value={shortcut}>
-          <DebugMove name={name} fn={fn} />
-        </KeyboardShortcut>
+        <DebugMove key={name} name={name} fn={fn} shortcut={shortcut} />
       );
     }
 

diff --git a/src/client/debug/debug.test.js b/src/client/debug/debug.test.js
@@ -9,7 +9,12 @@
 import React from 'react';
 import { restore } from '../../core/action-creators';
 import { createStore } from 'redux';
-import { Debug, DebugMove, KeyboardShortcut } from './debug.js';
+import {
+  Debug,
+  DebugMove,
+  DebugMoveArgField,
+  KeyboardShortcut,
+} from './debug.js';
 import Mousetrap from 'mousetrap';
 import Enzyme from 'enzyme';
 import Adapter from 'enzyme-adapter-react-16';
@@ -76,15 +81,17 @@ test('basic', () => {
 
 test('parse arguments', () => {
   const spy = jest.fn();
-  const root = Enzyme.mount(<DebugMove name="test" fn={spy} />);
+  const root = Enzyme.mount(<DebugMove name="test" fn={spy} shortcut="e" />);
 
-  root.instance().span.innerText = '1,2';
-  root.instance().onSubmit();
+  root.instance().onSubmit('1,2');
   expect(spy.mock.calls[0]).toEqual([1, 2]);
 
-  root.instance().span.innerText = '3, unknown, 4';
-  root.instance().onSubmit();
-  expect(spy.mock.calls[1]).toEqual([3, undefined, 4]);
+  root.instance().onSubmit('["a", "b"], ","');
+  expect(spy.mock.calls[1]).toEqual([['a', 'b'], ',']);
+
+  root.instance().onSubmit('3, unknown, 4');
+  expect(spy.mock.calls.length).toEqual(2);
+  expect(root.state().error).toEqual('ReferenceError: unknown is not defined');
 });
 
 test('KeyboardShortcut', () => {
@@ -96,18 +103,14 @@ test('KeyboardShortcut', () => {
 
 test('DebugMove', () => {
   const fn = jest.fn();
-  const root = Enzyme.mount(
-    <KeyboardShortcut value="e">
-      <DebugMove fn={fn} name="endTurn" />
-    </KeyboardShortcut>
-  );
+  const root = Enzyme.mount(<DebugMove fn={fn} name="endTurn" shortcut="e" />);
 
-  root.find(DebugMove).simulate('click');
+  root.simulate('click');
   root.find('.move span').simulate('keyDown', { key: 'Enter' });
 
   expect(fn.mock.calls.length).toBe(1);
 
-  root.find(DebugMove).simulate('click');
+  root.simulate('click');
   root.find('.move span').simulate('keydown', { key: '1' });
   root.find('.move span').simulate('keydown', { key: 'Enter' });
 
@@ -117,18 +120,18 @@ test('DebugMove', () => {
 test('escape blurs DebugMove', () => {
   const root = Enzyme.mount(
     <KeyboardShortcut value="e">
-      <DebugMove fn={jest.fn()} name="endTurn" />
+      <DebugMoveArgField onSubmit={jest.fn()} name="endTurn" />
     </KeyboardShortcut>
   );
 
   expect(root.state().active).toBe(false);
 
-  root.find(DebugMove).simulate('click');
+  root.find(DebugMoveArgField).simulate('click');
   expect(root.state().active).toBe(true);
   root.find('.move span').simulate('keydown', { key: 'Escape' });
   expect(root.state().active).toBe(false);
 
-  root.find(DebugMove).simulate('click');
+  root.find(DebugMoveArgField).simulate('click');
   expect(root.state().active).toBe(true);
   root.find('.move span').simulate('blur');
   expect(root.state().active).toBe(false);