Adding I18n string tracking decorator #36493
Conversation
| @@ -8,4 +8,9 @@ | |||
| */ | |||
| // locale for applab | |||
|
|
|||
| export default window.locales.applab_locale; | |||
There was a problem hiding this comment.
I wonder why this file doesn't use safeLoadLocale...
There was a problem hiding this comment.
probably an oversight on my part when I originally implemented that helper. I certainly don't recall intentionally excluding applab
| } | ||
|
|
||
| export default function localeWithI18nStringTracker(locale, source) { | ||
| if (!experiments.isEnabled(experiments.I18N_TRACKING)) { |
There was a problem hiding this comment.
I'm not against it but why do we need an experiment here? We're not uploading anything yet, plus this flag needs a deploy to change.
There was a problem hiding this comment.
This is for protection in case this wrapper does some how screw things up when deployed to the wild. I did try testing on all the level types, but I'm trying to be extra careful since this is a wide impacting change. I'd rather do another deploy to turn on the feature rather than an emergency deploy to turn it off.
| @@ -8,4 +8,9 @@ | |||
| */ | |||
| // locale for applab | |||
|
|
|||
| export default window.locales.applab_locale; | |||
There was a problem hiding this comment.
probably an oversight on my part when I originally implemented that helper. I certainly don't recall intentionally excluding applab
apps/src/util/i18nStringTracker.js
Outdated
| // Iterates each function in the given locale object and creates a wrapper function. | ||
| Object.keys(locale).forEach(function(stringKey, index) { | ||
| localeWithTracker[stringKey] = function(d) { | ||
| let value = locale[stringKey](d); |
There was a problem hiding this comment.
nit:
| let value = locale[stringKey](d); | |
| const value = locale[stringKey](d); |
apps/src/util/i18nStringTracker.js
Outdated
| return locale; | ||
| } | ||
|
|
||
| let localeWithTracker = {}; |
There was a problem hiding this comment.
nit:
| let localeWithTracker = {}; | |
| const localeWithTracker = {}; |
| // A map of "string source" -> "list of strings used" | ||
| // For example: window.stringUsageSources['maze'] = ['level_1.instruction.header'] | ||
| // The data recorded in this will periodically be uploaded. | ||
| if (!window.stringUsageSources) { |
There was a problem hiding this comment.
what's the motivation behind attaching this to the global window object?
There was a problem hiding this comment.
I just wanted somewhere to buffer them and have evidence that it's intercepting the string usage and logging it.
In a follow-up PR, where I actually use the buffer where the string usage is logged, I will re-evaluate what the best way to implement this buffer is. I will follow-up with you offline to see if you have any recommendations.
daynew
force-pushed
the
fnd-1243-javascript-string-tracking-decorator
branch
from
35444c8
to
f3f8331
Compare
As a code.org developer, I want to record what I18n strings we are using in our Javascript code.
This PR adds a decorator to our i18n strings (*_locale.js files) which stores the string key and the file it came from to
window.stringUsageSources. The records stored in this object will periodically be uploaded by a worker thread to our webservers and uploaded to Firehose (the worker thread will be implemented in a future PR).i18nStringTracker.jswhich decorates a given *_locale.js object so its usage can be recorded.window.stringUsageSourcesExample
window.stringUsageSourcesLinks
Testing story
window.locales.<locale_file>towindow.stringUsageSourcesReviewer Checklist: