From 8a33113c03120dd823fcfb915308f9a9021c412c Mon Sep 17 00:00:00 2001 From: zepumph Date: Mon, 23 Sep 2019 13:12:20 -0800 Subject: [PATCH 1/4] remove TODO, https://github.com/phetsims/chipper/issues/779 --- js/common/ChipperStringUtils.js | 1 - 1 file changed, 1 deletion(-) diff --git a/js/common/ChipperStringUtils.js b/js/common/ChipperStringUtils.js index 89848b626..e0fb724cb 100644 --- a/js/common/ChipperStringUtils.js +++ b/js/common/ChipperStringUtils.js @@ -132,7 +132,6 @@ if ( element.hasOwnProperty( 'value' ) ) { // remove leading/trailing whitespace, see chipper#619. Do this before addDirectionalFormatting - // TODO: some a11y strings have leading/trailing whitespace purposefully, perhaps we should formalize that somehow, https://github.com/phetsims/chipper/issues/779 element.value = element.value.trim(); element.value = ChipperStringUtils.addDirectionalFormatting( element.value, isRTL ); } From 79429ef5bb74f157bb4b595a2f55cfdf98ce664e Mon Sep 17 00:00:00 2001 From: zepumph Date: Mon, 23 Sep 2019 13:14:29 -0800 Subject: [PATCH 2/4] add typedef and documentation to string map structure, https://github.com/phetsims/chipper/issues/786 --- js/common/ChipperStringUtils.js | 39 +++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/js/common/ChipperStringUtils.js b/js/common/ChipperStringUtils.js index e0fb724cb..dcf89e4b1 100644 --- a/js/common/ChipperStringUtils.js +++ b/js/common/ChipperStringUtils.js @@ -118,17 +118,16 @@ /** * Recurse through a string file and format each string value appropriately - * @param {Object.} stringsMap - if "intermediary", then recurse to - * find more nested keys + * @param {StringMapNode} stringMapNode * @param {boolean} isRTL - is right to left language * @public */ - formatStringValues: function( stringObject, isRTL ) { - for ( const stringKey in stringObject ) { - if ( stringObject.hasOwnProperty( stringKey ) ) { + formatStringValues: function( stringMapNode, isRTL ) { + for ( const stringKey in stringMapNode ) { + if ( stringMapNode.hasOwnProperty( stringKey ) ) { // This will either have a "value" key, or be an object with keys that will eventually have 'value' in it - const element = stringObject[ stringKey ]; + const element = stringMapNode[ stringKey ]; if ( element.hasOwnProperty( 'value' ) ) { // remove leading/trailing whitespace, see chipper#619. Do this before addDirectionalFormatting @@ -149,7 +148,7 @@ * 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 {StringMap} map - where an "intermediate" Object should hold nested strings * @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 @@ -177,8 +176,8 @@ /** * Call a function on each string object in a string map. Recursively dive into each object that doesn't have a * `value` to find nested string objects too. - * @param {Object.} map - string map, like a loaded JSON strings file - * @param {function(key:string, {value:string})} func + * @param {StringMap} map - string map, like a loaded JSON strings file + * @param {function(key:string, StringObject)} func * @public */ forEachString( map, func ) { @@ -189,8 +188,8 @@ /** * This implementation function helps keep a better api for `forEachString`. * @param {string} keySoFar - as we recurse down, build up a string of the key separated with dots. - * @param {Object} map - string key map - * @param {function(key:string, {value:string})} func + * @param {StringMapNode} map + * @param {function(key:string, StringObject)} func */ const forEachStringImplementation = ( keySoFar, map, func ) => { for ( const key in map ) { @@ -207,6 +206,24 @@ } }; + /** + * @typedef {Object} StringMapNode + * @property {StringMapNode} * - A key that stores a StringMapNode inside this one. + */ + /** + * @typedef {Object} StringObject + * An object that has a "value" field that holds the string. It can still include more nested `StringObject`s. + * Each StringMapNode should have at least one StringObject nested inside it. + * @extends {StringMapNode} + * @property {string} value - the value key is used in + */ + /** + * @typedef {Object.>} StringMap + * @extends {StringMapNode} + * A string map can be either a flat map of StringObject (see the output of CHIPPER/getStringMap), or can be a nested + * Object with StringObjects throughout the object structure (as supported in English JSON string files). + */ + // browser require.js-compatible definition if ( typeof define !== 'undefined' ) { define( function() { From 229132439ad3cc6f5c84e2237fa5ce0a94c715b6 Mon Sep 17 00:00:00 2001 From: zepumph Date: Mon, 23 Sep 2019 14:16:36 -0800 Subject: [PATCH 3/4] formatStringValues implementation to use forEachString, bug fix in forEachString to ignore arrays, https://github.com/phetsims/rosetta/issues/193 --- js/common/ChipperStringUtils.js | 33 +++++++++++++-------------------- js/requirejs-plugins/string.js | 6 ++++++ 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/js/common/ChipperStringUtils.js b/js/common/ChipperStringUtils.js index dcf89e4b1..bccd1e83b 100644 --- a/js/common/ChipperStringUtils.js +++ b/js/common/ChipperStringUtils.js @@ -118,29 +118,17 @@ /** * Recurse through a string file and format each string value appropriately - * @param {StringMapNode} stringMapNode + * @param {StringMap} stringMap * @param {boolean} isRTL - is right to left language * @public */ - formatStringValues: function( stringMapNode, isRTL ) { - for ( const stringKey in stringMapNode ) { - if ( stringMapNode.hasOwnProperty( stringKey ) ) { - - // This will either have a "value" key, or be an object with keys that will eventually have 'value' in it - const element = stringMapNode[ stringKey ]; - if ( element.hasOwnProperty( 'value' ) ) { - - // remove leading/trailing whitespace, see chipper#619. Do this before addDirectionalFormatting - element.value = element.value.trim(); - element.value = ChipperStringUtils.addDirectionalFormatting( element.value, isRTL ); - } - else { - - // Recurse a level deeper - ChipperStringUtils.formatStringValues( element, isRTL ); - } - } - } + formatStringValues: function( stringMap, isRTL ) { + ChipperStringUtils.forEachString( stringMap, ( key, stringObject ) => { + + // remove leading/trailing whitespace, see chipper#619. Do this before addDirectionalFormatting + stringObject.value = stringObject.value.trim(); + stringObject.value = ChipperStringUtils.addDirectionalFormatting( stringObject.value, isRTL ); + } ); }, /** @@ -196,6 +184,11 @@ if ( map.hasOwnProperty( key ) ) { const nextKey = keySoFar ? `${keySoFar}.${key}` : key; // don't start with period, assumes '' is falsey const stringObject = map[ key ]; + + // no need to support arrays in the string map, for example stringObject.history in locale specific files. + if ( Array.isArray( stringObject ) ) { + return; + } if ( stringObject.value ) { func( nextKey, stringObject ); } diff --git a/js/requirejs-plugins/string.js b/js/requirejs-plugins/string.js index 699156fa5..8687def88 100644 --- a/js/requirejs-plugins/string.js +++ b/js/requirejs-plugins/string.js @@ -54,6 +54,12 @@ define( require => { // Cache miss: load the file parse, enter into cache and return it text.get( url, loadedText => { + + // the cache didn't exist before the text load, but it may exist now. + if ( cache[ url ] ) { + return callback( cache[ url ] ); + } + const parsed = JSON.parse( loadedText ); const isRTL = localeInfo[ phet.chipper.queryParameters.locale ].direction === 'rtl'; From 0c33bd65025e372e0fa6877e6118c3b41c47fa81 Mon Sep 17 00:00:00 2001 From: zepumph Date: Mon, 23 Sep 2019 14:33:01 -0800 Subject: [PATCH 4/4] cache string file loading before the first has loaded, https://github.com/phetsims/chipper/issues/797 --- js/requirejs-plugins/string.js | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/js/requirejs-plugins/string.js b/js/requirejs-plugins/string.js index 8687def88..e00e6ef41 100644 --- a/js/requirejs-plugins/string.js +++ b/js/requirejs-plugins/string.js @@ -28,10 +28,15 @@ define( require => { // constants // Cache the loaded strings so they only have to be read once through file.read (for performance) - // Object.>} + // Object.} - see ChipperStringUtils for typedef of StringMap // Where stringValueObject is the value of each key in string json files. const cache = {}; + // {Object.} - keep track of actions to trigger once the first load comes back for that + // url. This way there aren't many text.get calls kicked off before the first. + // can come back with text. + const urlsCurrentlyBeingLoaded = {}; + // {string|null} - See documentation of stringTest query parameter in initialize-globals.js const stringTest = ( typeof window !== 'undefined' && window.phet.chipper.queryParameters.stringTest ) ? window.phet.chipper.queryParameters.stringTest : @@ -50,22 +55,28 @@ define( require => { if ( cache[ url ] ) { callback( cache[ url ] ); } + else if ( urlsCurrentlyBeingLoaded[ url ] ) { + + // this url is currently being loaded, so don't kick off another `text.get()`. + urlsCurrentlyBeingLoaded[ url ].push( () => callback( cache[ url ] ) ); + } else { + urlsCurrentlyBeingLoaded[ url ] = []; + // Cache miss: load the file parse, enter into cache and return it text.get( url, loadedText => { - // the cache didn't exist before the text load, but it may exist now. - if ( cache[ url ] ) { - return callback( cache[ url ] ); - } - const parsed = JSON.parse( loadedText ); const isRTL = localeInfo[ phet.chipper.queryParameters.locale ].direction === 'rtl'; - ChipperStringUtils.formatStringValues( parsed, isRTL ); cache[ url ] = parsed; + + // clear the entries added during the async loading process + urlsCurrentlyBeingLoaded[ url ] && urlsCurrentlyBeingLoaded[ url ].forEach( action => action && action() ); + delete urlsCurrentlyBeingLoaded[ url ]; + callback( cache[ url ] ); }, errorBack, { accept: 'application/json' } ); }