Problems with grunt generate-development-strings

[pixelzoom]

For phetsims/ph-scale#258, I needed to remove a string from ph-scale-strings_en.json, because it was now obsolete and did not appear in the sim. But after running grunt modulify, the string was still appearing in Studio. This led to much head scratching. @samreid and I finally figured out that Studio was also loading babel/_generated_development_strings/ph-scale_all.json, and that we needed to also run grunt generate-development-strings -- which is also apparently run as part of nightly tasks.

Imo having a redundant source of strings like _generated_development_strings is a really bad idea. And not updating the redudnant source of strings when grunt modulify is run make it even worse. But if we must, then this info needs to be in some documentation, maybe how-to-change-a-string-key.md.

[samreid]

Thanks, reassigning to @jbphet and @liammulh who appear to be the main contributors to that MD file.

[jbphet]

@pixelzoom said above:

Imo having a redundant source of strings like _generated_development_strings is a really bad idea.

@samreid then said:

[R]eassigning to @jbphet and @liammulh who appear to be the main contributors to that MD file.

@samreid did most of the work on this under #1308. I recall making the same argument at the time that @pixelzoom is making, i.e. that having redundant sources of information is likely going to cause problems. And it is. So I'm going to hit this one back into @samreid's court for further consideration. I proposed the alternative of having a list of supported locales for each sim and loading just those files so that we don't run into timeouts in #1308 (comment). Perhaps we should reconsider that approach.

[samreid]

To investigate this issue, I ran performance testing comparing loading the non-conglomerate strings vs the conglomerate strings. To load the non-conglomerate strings (from each ground-truth file in babel), I first enumerated the locales CCK is translated into. Then I turned them into a query string like http://localhost/main/circuit-construction-kit-dc/circuit-construction-kit-dc_en.html?brand=phet&ea&debugger&locales=en,am,ar,ar_MA,az,be,bn,bs,ca,cs,cy,da,de,el,es,es_MX,es_PE,et,eu,fa,fi,fo,fr,gu,ha,hi,hr,ht,hu,hy,ig,in,is,it,iw,ja,ka,kk,kn,ko,ku,ku_TR,lg,lt,lv,mi,mn,mr,ms,mt,nb,nl,nn,ny,om,pl,pt,pt_BR,ro,ru,rw,sk,sl,sq,sr,st,sv,sw,ta,te,tg,th,tk,tr,tw,uk,ur,uz,vi,yo,zh_CN,zh_TW because those are loaded one at a time from the actual strings files (not from a conglomerate).

I ran that once as a warmup, then performance tested time from refresh to LCP (largest contentful paint). My tests are run on macbook air m1 running Chrome and http-server. The times were:
2579.5 ms
2659.1 ms
2773.1 ms
I also observed some 404s since some repos did not have strings for the languages at all (like bamboo).

Then I ran the same test with ?locales=*. I ran it once as a warmup then tested 3x to see the LCP. Results:
2107.8 ms
2152.3 ms
2168.0 ms

These averages differ by 527ms, which is more than half a second.

In my opinion, it is acceptable to have redundant information to get such a significant speed boost in our development. This speeds up sim development iteration, API file generation and CT, and phettest.

We would probably see a speed boost by listing which repos have which locales (so it would know not to load Bamboo/fr, for example), but then we would have the problem that the locale lists are redundant information to deal with, and need to be kept in sync, and it would still not be as fast as loading conglomerate files.

In summary, I feel a quick time-to-launch for our unbuilt sims is preferable to removing the redundant conglomerate files. I would recommend sharing the knowledge with the team about the conglomerate files, so that devs are aware and do not get surprised by this redundancy. If a developer needs to run a test that skips the conglomerate string file, they can do so by specifying ?locales=..... (list all the locales).

I would also like to update that file when grunt modulify and grunt update are run--so I'll look into that next.

[samreid]

I made it so that grunt update and grunt modulify regenerate the string conglomerate file for that repo. And I documented the need to do so in how-to-change-a-string-key.md. @pixelzoom can you please review?

[pixelzoom]

I don't see any problem with the commits. There should probably be a PSA so that devs are aware that grunt update and grunt modulify now behave differently. I see there was already confusion about that in Slack#developer this morning.

[samreid]

I sent a PSA to #dev-public that read:

Greetings developers, please be aware that grunt update and grunt modulify now generate a “conglomerate string file” that makes it possibly to quickly load many localizations when running in unbuilt mode. Also note: this is the first time that grunt update (unfortunately) changes files outside of the sim directory. Please also be aware the conglomerate string files redundantly specify the sim strings. The “en” file has special precedence and is not loaded from the conglomerate, but other strings are. And if you delete/rename a string from the _en.json file, please run grunt update to update the conglomerate (even though english strings are not loaded from it, stale keys can leak into studio). Full details in #1369

Anything else for this issue? Please close if all is well.