From 051c1d1a5e6e549f8c77a165545e927abf19da32 Mon Sep 17 00:00:00 2001 From: samreid Date: Mon, 23 Sep 2019 13:12:14 -0600 Subject: [PATCH] Simplify procedure for getting strings from loaded files, see https://github.com/phetsims/chipper/issues/786 --- js/common/ChipperStringUtils.js | 51 +++++++++++---------------------- js/grunt/getStringMap.js | 33 +++++++-------------- js/requirejs-plugins/string.js | 15 ++++++---- 3 files changed, 36 insertions(+), 63 deletions(-) diff --git a/js/common/ChipperStringUtils.js b/js/common/ChipperStringUtils.js index aa65f84d0..89848b626 100644 --- a/js/common/ChipperStringUtils.js +++ b/js/common/ChipperStringUtils.js @@ -146,50 +146,33 @@ }, /** - * Given a key, get the appropriate string from the "map" object. This method is called during requirejs mode from - * the string plugin and during the build via 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 + * Given a key, get the appropriate string from the "map" object, or null if the key does not appear in the map. + * This method is called during requirejs mode from the string plugin and during the build via CHIPPER/getStringMap. + * 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.} 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 + * @param {string} key - like `FRICTION/friction.title` or using nesting like `a11y.nested.string.here` + * @returns {string|null} - the string value of the key, or null if the key does not appear in the map * @throws {Error} - if the key doesn't hold a string value in the map * @public */ getStringFromMap( map, key ) { - // 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; - } - - // 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}` ); - } - - // 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 + throw new Error( 'getStringFromMap key should not have REPO/' ); } - const string = _.at( map, key )[ 0 ]; - if ( string === undefined || string.value === undefined ) { - throw new Error( `no entry for nested string key: ${key}` ); + // Lodash gives precedence to "key1.key2" over "key1:{key2}", so we do too. + const result = _.at( map, key )[ 0 ]; + if ( result ) { + if ( result.value === undefined ) { + throw new Error( `no value for string: ${key}` ); + } + return result.value; } - return string.value; + + // They key does not appear in the map + return null; }, /** diff --git a/js/grunt/getStringMap.js b/js/grunt/getStringMap.js index b1cba57e1..b266b7c29 100644 --- a/js/grunt/getStringMap.js +++ b/js/grunt/getStringMap.js @@ -31,6 +31,8 @@ module.exports = function( locales, phetLibs ) { // Get metadata of repositories that we want to load strings from (that were referenced in the sim) const stringRepositories = []; // { name: {string}, path: {string}, requirejsNamespace: {string} } + + // TODO: Use map/unique to get repository names for ( const stringKey in global.phet.chipper.strings ) { const repositoryName = global.phet.chipper.strings[ stringKey ].repositoryName; @@ -78,17 +80,11 @@ module.exports = function( locales, phetLibs ) { grunt.log.debug( `missing string file: ${stringsFilename}` ); fileContents = {}; } - const fileMap = stringFilesContents[ repository.name ][ locale ] = {}; // Format the string values ChipperStringUtils.formatStringValues( fileContents, isRTL ); - // Loop through all top level keys and add the repo prefix, since fileMap is cross repo for all the sim's strings. - for ( const stringKeyMissingPrefix in fileContents ) { - - // Add the requirejs namespaces (eg, JOIST) to the key - fileMap[ repository.requirejsNamespace + '/' + stringKeyMissingPrefix ] = fileContents[ stringKeyMissingPrefix ]; - } + stringFilesContents[ repository.name ][ locale ] = fileContents; } ); } ); @@ -100,24 +96,15 @@ module.exports = function( locales, phetLibs ) { stringMap[ locale ] = {}; for ( const stringKey in global.phet.chipper.strings ) { - const repositoryName = global.phet.chipper.strings[ stringKey ].repositoryName; - - // English fallback - const fallbackStringFileContents = stringFilesContents[ repositoryName ][ fallbackLocale ]; - - // This method will recurse if needed to find nested string values as well. - stringMap[ locale ][ stringKey ] = ChipperStringUtils.getStringFromMap( fallbackStringFileContents, stringKey ); + const stringMetadata = global.phet.chipper.strings[ stringKey ]; + const repositoryName = stringMetadata.repositoryName; + const key = stringMetadata.key; // Extract 'value' field from non-fallback (babel) strings file, and overwrites the default if available. - if ( locale !== fallbackLocale && - stringFilesContents[ repositoryName ] && - stringFilesContents[ repositoryName ][ locale ] && - stringFilesContents[ repositoryName ][ locale ][ stringKey ] && - - // if the string in rosetta is empty we want to use the fallback english string - stringFilesContents[ repositoryName ][ locale ][ stringKey ].value.length > 0 ) { - stringMap[ locale ][ stringKey ] = ChipperStringUtils.getStringFromMap( stringFilesContents[ repositoryName ][ locale ], stringKey ); - } + const value = ChipperStringUtils.getStringFromMap( stringFilesContents[ repositoryName ][ locale ], key ) || + ChipperStringUtils.getStringFromMap( stringFilesContents[ repositoryName ][ fallbackLocale ], key ); + + stringMap[ locale ][ stringKey ] = value; } } ); diff --git a/js/requirejs-plugins/string.js b/js/requirejs-plugins/string.js index afb26f16f..699156fa5 100644 --- a/js/requirejs-plugins/string.js +++ b/js/requirejs-plugins/string.js @@ -66,14 +66,16 @@ define( require => { }; /** - * - * @param {function} onload see doc for load() + * Only called in requirejs mode * @param {Object} fileContents - loaded from a string file * @param {string} key - the key of the string to be loaded - * @param {string} fileURL - for better error messaging */ const getStringFromFileContents = ( fileContents, key ) => { - return window.phet.chipper.mapString( ChipperStringUtils.getStringFromMap( fileContents, key ), stringTest ); + const stringFromMap = ChipperStringUtils.getStringFromMap( fileContents, key ); + if ( stringFromMap === null ) { + throw new Error( `String not found: ${key}` ); + } + return window.phet.chipper.mapString( stringFromMap, stringTest ); }; return { @@ -139,7 +141,8 @@ define( require => { requirejsNamespace: requirejsNamespace, // 'SOME_SIM' requirePath: requirePath, // '/Users/something/phet/git/some-sim/js' repositoryPath: repositoryPath, // '/Users/something/phet/git/some-sim' - repositoryName: repositoryName // 'some-sim' + repositoryName: repositoryName, // 'some-sim' + key: key // 'string.title }; // tell require.js we're done processing @@ -184,7 +187,7 @@ define( require => { getWithCache( localeSpecificPath, parsed => { // Combine the primary and fallback strings into one object hash. - const parsedStrings = _.extend( parsedFallbackStrings, parsed ); + const parsedStrings = _.extend( {}, parsedFallbackStrings, parsed ); onload( getStringFromFileContents( parsedStrings, key ) ); },