Skip to content

Commit

Permalink
MDL-4915 templates: Improve JS string handling in templates
Browse files Browse the repository at this point in the history
Backported from MDL-53667 to make cherry-picking easier across branches.

The native String.replace method in extremely slow when we are
dealing with a large string and large quantity of strings to replace.
This new solution walks through the string looking for placeholders
to replace.
  • Loading branch information
Frederic Massart authored and Damyon Wiese committed Jul 19, 2016
1 parent 2772b59 commit d2b890c
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 34 deletions.
2 changes: 1 addition & 1 deletion lib/amd/build/templates.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

122 changes: 89 additions & 33 deletions lib/amd/src/templates.js
Expand Up @@ -31,9 +31,11 @@ define([ 'core/mustache',
'core/url',
'core/config',
'core/localstorage',
'core/event'
'core/event',
'core/yui',
'core/log'
],
function(mustache, $, ajax, str, notification, coreurl, config, storage, event) {
function(mustache, $, ajax, str, notification, coreurl, config, storage, event, Y, Log) {

// Private variables and functions.

Expand Down Expand Up @@ -222,13 +224,77 @@ define([ 'core/mustache',
js = requiredJS.join(";\n");
}

var i = 0;

for (i = 0; i < strings.length; i++) {
js = js.replace('{{_s' + i + '}}', strings[i]);
}
// Re-render to get the final strings.
return js;
return treatStringsInContent(js, strings);
};

/**
* Treat strings in content.
*
* The purpose of this method is to replace the placeholders found in a string
* with the their respective translated strings.
*
* Previously we were relying on String.replace() but the complexity increased with
* the numbers of strings to replace. Now we manually walk the string and stop at each
* placeholder we find, only then we replace it. Most of the time we will
* replace all the placeholders in a single run, at times we will need a few
* more runs when placeholders are replaced with strings that contain placeholders
* themselves.
*
* @param {String} content The content in which string placeholders are to be found.
* @param {Array} strings The strings to replace with.
* @return {String} The treated content.
*/
var treatStringsInContent = function(content, strings) {
var pattern = /{{_s\d+}}/,
treated,
index,
strIndex,
walker,
char,
strFinal;

do {
treated = '';
index = content.search(pattern);
while (index > -1) {

// Copy the part prior to the placeholder to the treated string.
treated += content.substring(0, index);
content = content.substr(index);
strIndex = '';
walker = 4; // 4 is the length of '{{_s'.

// Walk the characters to manually extract the index of the string from the placeholder.
char = content.substr(walker, 1);
do {
strIndex += char;
walker++;
char = content.substr(walker, 1);
} while (char != '}');

// Get the string, add it to the treated result, and remove the placeholder from the content to treat.
strFinal = strings[parseInt(strIndex, 10)];
if (typeof strFinal === 'undefined') {
Log.debug('Could not find string for pattern {{_s' + strIndex + '}}.');
strFinal = '';
}
treated += strFinal;
content = content.substr(6 + strIndex.length); // 6 is the length of the placeholder without the index: '{{_s}}'.

// Find the next placeholder.
index = content.search(pattern);
}

// The content becomes the treated part with the rest of the content.
content = treated + content;

// Check if we need to walk the content again, in case strings contained placeholders.
index = content.search(pattern);

} while (index > -1);

return content;
};

/**
Expand Down Expand Up @@ -260,35 +326,25 @@ define([ 'core/mustache',
}

if (requiredStrings.length > 0) {
str.get_strings(requiredStrings).done(
function(strings) {
var i;

// Why do we not do another call the render here?
//
// Because that would expose DOS holes. E.g.
// I create an assignment called "{{fish" which
// would get inserted in the template in the first pass
// and cause the template to die on the second pass (unbalanced).
for (i = 0; i < strings.length; i++) {
result = result.replace('{{_s' + i + '}}', strings[i]);
}
deferred.resolve(result.trim(), getJS(strings));
}
).fail(
function(ex) {
deferred.reject(ex);
}
);
str.get_strings(requiredStrings)
.then(function(strings) {

// Why do we not do another call the render here?
//
// Because that would expose DOS holes. E.g.
// I create an assignment called "{{fish" which
// would get inserted in the template in the first pass
// and cause the template to die on the second pass (unbalanced).

result = treatStringsInContent(result, strings);
deferred.resolve(result, getJS(strings));
})
.fail(deferred.reject);
} else {
deferred.resolve(result.trim(), getJS([]));
}
}
).fail(
function(ex) {
deferred.reject(ex);
}
);
).fail(deferred.reject);
return deferred.promise();
};

Expand Down

0 comments on commit d2b890c

Please sign in to comment.