fix: correctly format several values in browser-repl (#395)

Update the formatting for several values in the browser REPL.
Since fixing this for Date objects consistently enabled doing
straightforward fixes for a few other issues, this addresses:

- COMPASS-4440 (Consistently formatting `Date` objects)
- COMPASS-4441 (Ignoring `undefined` results)
- MONGOSH-410 (`sh.status()` formatting)
- COMPASS-4465 (`ObjectId` formatting in nested objects)

Also include a minor change to the evergreen config that
may or may not have resolved a surprising failure when
this PR was first opened.

Author: addaleax

.evergreen/.install_node

@@ -1,7 +1,8 @@
+set -e
 export NODE_JS_VERSION='12.18.4'
+export NVM_DIR="$HOME/.nvm"
 
 curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.3/install.sh | bash
-export NVM_DIR="$HOME/.nvm"
 
 echo "Setting NVM environment home: $NVM_DIR"
 [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"

.evergreen/.setup_env

@@ -1,3 +1,4 @@
+set -e
 export NVM_DIR="$HOME/.nvm"
 echo "Setting NVM environment home: $NVM_DIR"
 [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"

packages/browser-repl/src/components/shell-output-line.spec.tsx

@@ -149,7 +149,7 @@ describe('<ShellOutputLine />', () => {
       }
     }} />);
 
-    expect(wrapper.text()).to.include('---');
+    expect(wrapper.find('hr')).to.have.lengthOf(1);
     expect(wrapper.text()).to.include('metadata');
   });
 

packages/browser-repl/src/components/shell-output-line.tsx

@@ -13,9 +13,9 @@ import { ShowCollectionsOutput } from './types/show-collections-output';
 import { CursorOutput } from './types/cursor-output';
 import { CursorIterationResultOutput } from './types/cursor-iteration-result-output';
 import { ObjectOutput } from './types/object-output';
+import { StatsResultOutput } from './types/stats-result-output';
 import { SimpleTypeOutput } from './types/simple-type-output';
 import { ErrorOutput } from './types/error-output';
-import { inspect } from './utils/inspect';
 import { ShowProfileOutput } from './types/show-profile-output';
 
 const styles = require('./shell-output-line.less');
@@ -47,6 +47,10 @@ export class ShellOutputLine extends Component<ShellOutputLineProps> {
       return <pre>{value}</pre>;
     }
 
+    if (typeof value === 'string' && type !== null) {
+      return <SimpleTypeOutput value={value} raw />;
+    }
+
     if (this.isPrimitiveOrFunction(value)) {
       return <SimpleTypeOutput value={value} />;
     }
@@ -60,10 +64,7 @@ export class ShellOutputLine extends Component<ShellOutputLineProps> {
     }
 
     if (type === 'StatsResult') {
-      const res = Object.keys(value).map(c => {
-        return `${c}\n${inspect(value[c])}`;
-      }).join('\n---\n');
-      return <SimpleTypeOutput value={res} />;
+      return <StatsResultOutput value={value} />;
     }
 
     if (type === 'ListCommandsResult)') {

packages/browser-repl/src/components/shell-output.spec.tsx

@@ -17,6 +17,12 @@ describe('<ShellOutput />', () => {
     expect(wrapper.find(ShellOutputLine)).to.have.lengthOf(1);
   });
 
+  it('renders no output lines if only one with a value of undefined is passed', () => {
+    const line1: ShellOutputEntry = { type: 'output', value: undefined };
+    const wrapper = shallow(<ShellOutput output={[line1]} />);
+    expect(wrapper.find(ShellOutputLine)).to.have.lengthOf(0);
+  });
+
   it('pass the entry to the output line as prop', () => {
     const line1: ShellOutputEntry = { type: 'output', value: 'line 1' };
     const wrapper = shallow(<ShellOutput output={[line1]} />);

packages/browser-repl/src/components/shell-output.tsx

@@ -18,6 +18,6 @@ export class ShellOutput extends Component<ShellOutputProps> {
   };
 
   render(): JSX.Element[] {
-    return this.props.output.map(this.renderLine);
+    return this.props.output.filter(entry => entry.value !== undefined).map(this.renderLine);
   }
 }

packages/browser-repl/src/components/types/simple-type-output.tsx

@@ -5,15 +5,18 @@ import { inspect } from '../utils/inspect';
 
 interface SimpleTypeOutputProps {
   value: any;
+  raw?: boolean;
 }
 
 export class SimpleTypeOutput extends Component<SimpleTypeOutputProps> {
   static propTypes = {
-    value: PropTypes.any
+    value: PropTypes.any,
+    raw: PropTypes.bool
   };
 
   render(): JSX.Element {
-    return (<SyntaxHighlight code={inspect(this.props.value)} />);
+    const asString = this.props.raw ? this.props.value : inspect(this.props.value);
+    return (<SyntaxHighlight code={asString} />);
   }
 }
 

packages/browser-repl/src/components/types/stats-result-output.tsx

@@ -0,0 +1,27 @@
+import React, { Component } from 'react';
+import PropTypes from 'prop-types';
+import { ObjectOutput } from './object-output';
+
+interface StatsResultOutputProps {
+  value: Record<string, any>;
+}
+
+export class StatsResultOutput extends Component<StatsResultOutputProps> {
+  static propTypes = {
+    value: PropTypes.any
+  };
+
+  render(): JSX.Element {
+    const result: JSX.Element[] = [];
+    for (const [ key, value ] of Object.entries(this.props.value)) {
+      if (result.length > 0) {
+        result.push(<hr key={`${key}-separator`} />);
+      }
+      result.push(<div key={key}>
+        <h4>{key}</h4>
+        <ObjectOutput value={value} />
+      </div>);
+    }
+    return <div>{result}</div>;
+  }
+}

packages/browser-repl/src/components/utils/inspect.spec.ts

@@ -37,13 +37,46 @@ describe('inspect', () => {
         inspect(undefined)
       ).to.equal('undefined');
     });
+
+    it('inspects Dates', () => {
+      expect(
+        inspect(new Date('2020-11-06T14:26:29.131Z'))
+      ).to.equal('2020-11-06T14:26:29.131Z');
+    });
   });
 
   context('with BSON types', () => {
     it('inspects ObjectId', () => {
       expect(
         inspect(new bson.ObjectId('0000007b3db627730e26fd0b'))
-      ).to.equal('ObjectID("0000007b3db627730e26fd0b")');
+      ).to.equal('ObjectId("0000007b3db627730e26fd0b")');
+    });
+
+    it('inspects UUID', () => {
+      expect(
+        inspect(new bson.Binary('abcdefghiklmnopq', 4))
+      ).to.equal('UUID("61626364-6566-6768-696b-6c6d6e6f7071")');
+    });
+
+    it('inspects nested ObjectId', () => {
+      expect(
+        inspect({ p: new bson.ObjectId('0000007b3db627730e26fd0b') })
+      ).to.equal('{ p: ObjectId("0000007b3db627730e26fd0b") }');
+    });
+
+    it('inspects nested UUID', () => {
+      expect(
+        inspect({ p: new bson.Binary('abcdefghiklmnopq', 4) })
+      ).to.equal('{ p: UUID("61626364-6566-6768-696b-6c6d6e6f7071") }');
+    });
+
+    it('does not require BSON types to be instances of the current bson library', () => {
+      expect(
+        inspect({
+          _bsontype: 'ObjectID',
+          toHexString() { return '0000007b3db627730e26fd0b'; }
+        })
+      ).to.equal('ObjectId("0000007b3db627730e26fd0b")');
     });
   });
 
@@ -56,4 +89,13 @@ describe('inspect', () => {
       });
     });
   });
+
+  context('with frozen objects with _bsontype properties', () => {
+    expect(
+      () => inspect(Object.freeze({
+        _bsontype: 'ObjectID',
+        toHexString() { return '0000007b3db627730e26fd0b'; }
+      }))
+    ).not.to.throw;
+  });
 });

packages/browser-repl/src/components/utils/inspect.ts

@@ -1,50 +1,85 @@
 import { inspect as utilInspect } from 'util';
-type BSONBaseType = { _bsontype: string };
+import { bsonStringifiers } from '@mongosh/service-provider-core';
 
-const formatBsonType = (value: BSONBaseType): any => ({
-  inspect(): string {
-    return `${value._bsontype}(${(JSON.stringify(value))})`;
-  }
-});
+// At the time of writing, the Compass dist package contains what appear to be
+// 155 different copies of the 'bson' module. It is impractical to attach
+// our inspection methods to each of those copies individually, like we do when
+// we are inside cli-repl.
+// Instead, we look for values with a _bsontype property inside the object graph
+// before printing them here, and attach inspection methods to each of them
+// individually.
+// This is not particularly fast, but should work just fine for user-facing
+// interfaces like printing shell output in the browser.
 
-function isBsonType(value: any): boolean {
-  return !!(value && value._bsontype);
-}
+const customInspect = utilInspect.custom || 'inspect';
+const visitedObjects = new WeakSet();
 
-function isObject(value: any): boolean {
-  return !!(value && typeof value === 'object' && !Array.isArray(value));
+function tryAddInspect(obj: any, stringifier: (this: any) => string): void {
+  try {
+    Object.defineProperty(obj, customInspect, {
+      writable: true,
+      configurable: true,
+      enumerable: false,
+      value: function() {
+        try {
+          return stringifier.call(this);
+        } catch (err) {
+          console.warn('Could not inspect bson object', { obj: this, err });
+          return utilInspect(this, { customInspect: false });
+        }
+      }
+    });
+  } catch (err) {
+    console.warn('Could not add inspect key to object', { obj, err });
+  }
 }
 
-function formatProperty(value: any): any {
-  if (isObject(value) && isBsonType(value)) {
-    return formatBsonType(value);
+function isDate(value: any): boolean {
+  try {
+    Date.prototype.getTime.call(value);
+    return true;
+  } catch {
+    return false;
   }
-
-  return value;
 }
 
-function formatObject(object: any): any {
-  const viewObject: any = {};
-  for (const key of Object.keys(object)) {
-    viewObject[key] = formatProperty(object[key]);
+function attachInspectMethods(obj: any): void {
+  if ((typeof obj !== 'object' && typeof obj !== 'function') || obj === null) {
+    // Ignore primitives
+    return;
   }
-  return viewObject;
-}
 
-function toViewValue(value: any): any {
-  if (isBsonType(value)) {
-    return formatBsonType(value);
+  if (visitedObjects.has(obj)) {
+    return;
+  }
+  visitedObjects.add(obj);
+
+  // Traverse the rest of the object graph.
+  attachInspectMethods(Object.getPrototypeOf(obj));
+  const properties = Object.getOwnPropertyDescriptors(obj);
+  for (const { value } of Object.values(properties)) {
+    attachInspectMethods(value);
   }
 
-  if (isObject(value)) {
-    return formatObject(value);
+  // Add obj[util.inspect.custom] if it does not exist and we can provide it.
+  const bsontype = obj._bsontype;
+  const stringifier = bsonStringifiers[bsontype];
+  if (bsontype &&
+      stringifier &&
+      !(properties as any)[customInspect] &&
+      !Object.isSealed(obj)) {
+    tryAddInspect(obj, stringifier);
+  } else if (isDate(obj)) {
+    tryAddInspect(obj, function(this: Date): string {
+      return this.toISOString();
+    });
   }
-  return value;
 }
 
 export function inspect(value: any): string {
-  const viewValue = toViewValue(value);
-  const stringifiedValue = utilInspect(viewValue, {
+  attachInspectMethods(value);
+
+  const stringifiedValue = utilInspect(value, {
     customInspect: true,
     depth: 1000,
     breakLength: 0