rewrite string getting code for simplicity, fix reportUnusedStrings, #…

…786

js/common/ChipperConstants.js

@@ -11,7 +11,7 @@
 /* eslint-env browser, node */
 'use strict';
 
-(function() {
+( function() {
 
   var ChipperConstants = {
 
@@ -46,4 +46,4 @@
     module.exports = ChipperConstants;
   }
 
-})();
+} )();

js/common/ChipperStringUtilTests.js

@@ -0,0 +1,50 @@
+// Copyright 2019, University of Colorado Boulder
+
+/**
+ * Tests for ChipperStringUtils
+ * @author Michael Kauzmann (PhET Interactive Simulations)
+ */
+
+/* eslint-env node*/
+'use strict';
+
+const ChipperStringUtils = require( './ChipperStringUtils' );
+const qunit = require( 'qunit' );
+qunit.module( 'ChipperStringUtils' );
+
+
+qunit.test( 'forEachString', assert => {
+  const map1 = {
+    x: { value: 'x' },
+    y: {
+      value: 'y',
+      z: { value: 'z' }
+    },
+    intermediary: {
+      a: { value: 'a' },
+      b: { value: 'b' },
+      intermediary2: {
+        c: { value: 'c' }
+      }
+    }
+  };
+
+  let count = 0;
+  const expectedKeys = [
+    'x',
+    'y',
+    'y.z',
+    'intermediary.a',
+    'intermediary.b',
+    'intermediary.intermediary2.c'
+  ];
+  ChipperStringUtils.forEachString( map1, key => {
+    count++;
+    const keyIndex = expectedKeys.indexOf( key );
+    assert.ok( keyIndex >= 0, 'unexpected key:' + key );
+    expectedKeys.splice( keyIndex, 1 ); // just remove the single item
+  } );
+  assert.ok( expectedKeys.length === 0, 'all keys should be accounted for' );
+  assert.ok( count === 6, 'should be three string' );
+  assert.ok( true, 'success' );
+} );

js/common/ChipperStringUtils.js

@@ -12,13 +12,8 @@
 
 ( function() {
 
-  // constants
-  // This string key supports nested string objects, see https://github.com/phetsims/rosetta/issues/193
-  // NOTE: don't change this without consulting the duplication in the `string-require-statement-match.js` eslint rule.
-  const A11Y_STRING_KEY_NAME = 'a11y';
-
-  // Any string keys beginning with this marker support nested string values
-  const A11Y_KEY_MARKER = `${A11Y_STRING_KEY_NAME}.`;
+  // What divides the repo prefix from the rest of the string key, like `FRICTION/friction.title`
+  const NAMESPACE_PREFIX_DIVIDER = '/';
 
   var ChipperStringUtils = {
 
@@ -154,69 +149,45 @@
      * This method also supports being called from CHIPPER/getStringMap, which adds the REPO prefix to all string keys
      * in the map. This method supports recursing through keys that support string nesting. This method was
      * created to support nested string keys in https://github.com/phetsims/rosetta/issues/193
-     * @param {Object.<string, intermediate:Object|{value: string}>} map - where 'intermediate' should hold nested strings
+     * @param {Object.<string, Object|{value: string}>} map - where an "intermediate" Object should hold nested strings
      * @param {string} key - like `friction.title` or `FRICTION/friction.title` or using nesting like `a11y.nested.string.here`
      * @returns {string} - the string value of the key
      * @throws  {Error} - if the key doesn't hold a string value in the map
      */
     getStringFromMap( map, key ) {
-      let repoPrefix = '';
-      if ( key.indexOf( '/' ) >= 0 ) {
-        if ( key.match( /\//g ).length > 1 ) {
-          throw new Error( `more forward slashes than expected in key: ${key}` );
-        }
-        repoPrefix = key.split( '/' )[ 0 ];
-      }
 
-      // Normal case where the key holds an object with the string in the "value" key, i.e. "friction.title": { "value": "Friction" },
+      // Normal case where the map is "flat," and the key holds an object with the string in the "value" key, i.e. "friction.title": { "value": "Friction" },
       if ( map[ key ] !== undefined ) {
         if ( map[ key ].value === undefined ) {
           throw new Error( `no value entry for string key: ${key}` );
         }
         return map[ key ].value;
       }
 
-      // `key` begins with nested section marker in requirejs case, where there isn't a prefix repo, like `a11y.string`
-      // instead of `FRICTION/a11y.string`
-      else if ( key.indexOf( A11Y_KEY_MARKER ) === 0 ) {
-        if ( repoPrefix !== '' ) {
-          throw new Error( `unexpected repo prefex ${repoPrefix}` );
-        }
-
-        // access the nested object with a key like `a11y.my.string.is.all.the.way.down.here`
-        const string = _.at( map, key )[ 0 ];
-        if ( string === undefined || string.value === undefined ) {
-          throw new Error( `no entry for nested string key: ${key}` );
-        }
-
-        // TODO: recurse through each and assert that there is no "value" key in each nested object? (maybe)
-        // TODO: for example `a11y.my.string.is.all.the.way.value` should not have a key because
-        // TODO: `a11y.my.string.is.all.the.way.down.here.value` is a string value
-        // TODO: perhaps also do this for getStringMap case below, but maybe this is only needed here for requirejs mode
-        // TODO: https://github.com/phetsims/rosetta/issues/193
-        return string.value;
+      // If not directly accessing the top level of the map, then treat the map like a nested object
+      // Test for the presence of the namespace prefix, as that will dictate how the string is accessed
+      const dividerMatches = key.match( new RegExp( NAMESPACE_PREFIX_DIVIDER, 'g' ) );
+      if ( dividerMatches && dividerMatches.length > 1 ) {
+        throw new Error( `more forward slashes than expected in key: ${key}` );
       }
 
-      // This supports being called from CHIPPER/getStringMap with keys like `FRICTION/a11y.some.string.here`
-      else if ( repoPrefix && key.indexOf( `${repoPrefix}/${A11Y_KEY_MARKER}` ) === 0 ) {
-
-        // The first key in the map is like "FRICTION/a11y": { . . . }
-        const nestedStringsKey = `${repoPrefix}/${A11Y_STRING_KEY_NAME}`;
-        const nestedStrings = map[ nestedStringsKey ];
-
-        // access the nested object, remove the `FRICTION/a11y` piece because we already accessed that nested object above
-        const string = _.at( nestedStrings, key.replace( `${nestedStringsKey}.`, '' ) )[ 0 ];
-        if ( string === undefined || string.value === undefined ) {
-          throw new Error( `no entry for nested string key: ${key}` );
-        }
-
-        return string.value;
+      // In this case the map and key have a namespace prefix, but the map is using nested objects, so the following are
+      // needed:
+      // Take the string key `FRICTION/a11y.temperature.hotter
+      // The map looks like `FRICTION/a11y: { temperature: { hotter: { value: '. . .' } } }`
+      // So we need to peel off the first key
+      if ( key.indexOf( NAMESPACE_PREFIX_DIVIDER ) >= 0 ) {
+        const keyParts = key.split( '.' );
+        const firstKeyPart = keyParts.splice( 0, 1 );
+        map = map[ firstKeyPart ];
+        key = keyParts.join( '.' ); // this no longer has the first key in it
       }
-      else {
 
-        // It would be really strange for there to be no fallback for a certain string, that means it exists in the translation but not the original English
-        throw new Error( `no entry for string key: ${key}` );
+      const string = _.at( map, key )[ 0 ];
+      if ( string === undefined || string.value === undefined ) {
+        throw new Error( `no entry for nested string key: ${key}` );
       }
+      return string.value;
     },
 
     /**
@@ -228,13 +199,7 @@
      */
     forEachString( map, func ) {
       forEachStringImplementation( '', map, func );
-    },
-
-    /**
-     * @type {string}
-     * @pubic
-     */
-    A11Y_STRING_KEY_NAME: A11Y_STRING_KEY_NAME
+    }
   };
 
   /**
@@ -246,15 +211,14 @@
   const forEachStringImplementation = ( keySoFar, map, func ) => {
     for ( const key in map ) {
       if ( map.hasOwnProperty( key ) ) {
-        const nextKey = keySoFar ? `${keySoFar}.${key}` : key; // don't start with period
-        if ( map[ key ].value ) {
-          func( nextKey, map[ key ] );
+        const nextKey = keySoFar ? `${keySoFar}.${key}` : key; // don't start with period, assumes '' is falsey
+        const stringObject = map[ key ];
+        if ( stringObject.value ) {
+          func( nextKey, stringObject );
         }
-        else {
 
-          // recurse to the next level since this wasn't the `value` key
-          forEachStringImplementation( nextKey, map[ key ], func );
-        }
+        // recurse to the next level since if it wasn't the `value` key
+        key !== 'value' && forEachStringImplementation( nextKey, stringObject, func );
       }
     }
   };

js/grunt/chipperGlobals.js

@@ -58,7 +58,7 @@ module.exports = {
     assert( _.includes( ChipperConstants.BRANDS, brand ), 'Unknown brand in beforeBuild: ' + brand );
 
     global.phet.chipper.mipmapsToBuild = [];
-    global.phet.chipper.strings = {};
+    global.phet.chipper.strings = {}; // See string.js for when this is set; only when running requirejs optimizer.
     global.phet.chipper.brand = brand;
     global.phet.chipper.licenseEntries = {};
   }

js/grunt/getStringMap.js

@@ -26,7 +26,6 @@ const localeInfo = require( '../data/localeInfo' ); // Locale information
 module.exports = function( locales, phetLibs ) {
 
   const fallbackLocale = ChipperConstants.FALLBACK_LOCALE; // local const to improve readability
-
   assert( global.phet && global.phet.chipper && global.phet.chipper.strings, 'missing global.phet.chipper.strings' );
   assert( locales.indexOf( fallbackLocale ) !== -1, 'fallback locale is required' );
 
@@ -109,9 +108,7 @@ module.exports = function( locales, phetLibs ) {
       const stringsForFallbackLocale = repoStringMap[ repositoryName ][ fallbackLocale ];
 
       // This method will recurse if needed to find nested string values as well.
-      const fallbackString = ChipperStringUtils.getStringFromMap( stringsForFallbackLocale, stringKey, locale === 'en' );
-
-      stringMap[ locale ][ stringKey ] = fallbackString;
+      stringMap[ locale ][ stringKey ] = ChipperStringUtils.getStringFromMap( stringsForFallbackLocale, stringKey );
 
       // Extract 'value' field from non-fallback (babel) strings file, and overwrites the default if available.
       if ( locale !== fallbackLocale &&

js/grunt/reportUnusedStrings.js

@@ -26,45 +26,14 @@ module.exports = function( repo, requirejsNamespace ) {
   // get the strings for this sim
   const jsStrings = grunt.file.readJSON( `../${repo}/${repo}-strings_en.json` );
 
-  // iterate over the strings
-  for ( const key in jsStrings ) {
+  // global.phet.chipper.strings is initialized by the string plugin
+  const chipperStrings = global.phet.chipper.strings || {};
 
-    if ( jsStrings.hasOwnProperty( key ) ) {
+  ChipperStringUtils.forEachString( jsStrings, ( key, stringObject ) => {
 
-      const string = jsStrings[ key ].value;
-      const requireStringKey = requirejsNamespace + '/' + key;
-
-      // global.phet.chipper.strings is initialized by the string plugin
-      const chipperStrings = global.phet.chipper.strings || {};
-
-      /**
-       * Warn if the string is not used.
-       * @param {string} fullKey - with the `REPO/` included
-       * @param {string} key - just the key, no `REPO/`
-       * @param {string} value - the value of the string
-       */
-      const warnIfStringUnused = ( fullKey, key, value ) => {
-
-        // If this string was not added to the global chipperStrings, it was not required in the sim
-        if ( !chipperStrings.hasOwnProperty( fullKey ) ) {
-          grunt.log.warn( `Unused string: key=${key}, value=${value}` );
-        }
-      };
-
-      // for top level strings, warn if there is a `value` key in the string object
-      jsStrings[ key ].value && warnIfStringUnused( requireStringKey, key, string );
-
-      // support nesting into a11y strings
-      if ( key === ChipperStringUtils.A11Y_STRING_KEY_NAME ) {
-
-        const a11yStrings = jsStrings[ key ];
-
-        ChipperStringUtils.forEachString( a11yStrings, ( a11ySubKey, stringObject ) => {
-          const keyWithRepo = `${requireStringKey}.${a11ySubKey}`;
-          const fullKeyNoRepo = `${ChipperStringUtils.A11Y_STRING_KEY_NAME}.${a11ySubKey}`;
-          warnIfStringUnused( keyWithRepo, fullKeyNoRepo, stringObject.value );
-        } );
-      }
+    // If this string was not added to the global chipperStrings, it was not required in the sim
+    if ( !chipperStrings.hasOwnProperty( `${requirejsNamespace}/${key}` ) ) {
+      grunt.log.warn( `Unused string: key=${key}, value=${stringObject.value}` );
     }
-  }
+  } );
 };

js/requirejs-plugins/string.js

@@ -41,7 +41,7 @@ define( require => {
    * When running in requirejs mode, check to see if we have already loaded the specified file
    * Also parses it so that only happens once per file (instead of once per string key)
    * @param {string} url path for the string
-   * @param {function} callback callback when the check succeeds
+   * @param {function} callback callback when the check succeeds, returning the parsed JSON object
    * @param {function} errorBack callback for when an error occurred
    */
   const getWithCache = ( url, callback, errorBack ) => {
@@ -201,7 +201,7 @@ define( require => {
     },
 
     /**
-     * Write code that will look up the string in the compiled sim, used in the compilation step.  write is only used
+     * Write code that will look up the string in the compiled sim, used in the compilation step. `write` is only used
      * by the optimizer, and it only needs to be implemented if the plugin can output something that would belong in an
      * optimized layer.
      *

test/generalTests.js

@@ -0,0 +1,12 @@
+// Copyright 2019, University of Colorado Boulder
+
+/**
+ * launch point to load any tests located around the chipper repo. This is to support running `qunit` with no args
+ * from the top level of chipper, as is the recommended way to run chipper tests.
+ *
+ * @author Michael Kauzmann (PhET Interactive Simulations)
+ */
+
+'use strict';
+
+require( '../js/common/ChipperStringUtilTests' );