Safety for Fluent string dependencies

[jonathanolson]

I noted in slack to @jessegreenberg that fluent strings being able to depend on others can create a problem for common strings (or even... sim-specific strings between release branches):

Case 1:

• Sim Y is published (common code repo has string A)
• Common code introduces string B
• Sim X is published (common code repo has string A and string B)
• Translator translates sim X (having string A refer to string B, since they are in the same repo and both available in sim X) - that sim works.
• Something causes rebuild of sim Y --- it breaks! (string A refers to string B, which doesn't exist in its release branch). Presumably hard build failure.

Case 2:

• Sim Y is published (strings A and B exist, but sim Y ONLY uses string A)
• Sim X is published (uses both strings)
• Translator translates sim X (having string A refer to string B, since they are in the same repo and both available in sim X) - that sim works.
• Something causes rebuild of sim Y --- it potentially builds but without string B in the runtime, meaning it gets deployed, and now sim Y is runtime-broken because string A refers to string B which wouldn't be included.

As part of scenerystack/scenerystack#55, I'm not yet handling the string dependencies, but it would be similar.

I believe essentially each build would need to run fluent dependency tracking for every locale (instead of just English, as is done now), to create the set of strings that will be included.

Once this is a bit more figured out, I'll need to adjust SceneryStack to run this (and to include dependencies in every FluentConstant/FluentPattern).

[jonathanolson]

It also came up that because we are wrapping non-a11y strings in FluentConstant (when a legacy pattern isn't detected), these strings can:

1. Have references added in translations, e.g. { someOtherString }
2. or more seriously, have escaping issues if the English string doesn't have braces, but a translation does. NOTE this could work for a translator (when translating a sim that is older and doesn't use FluentConstant), but then would BREAK sims that are newer and wrap it in FluentConstant.

So to me, wrapping those strings in FluentConstant is a breaking change, and I'm nervous about release branches going out with SceneryPhetFluent in its current state. Marking as blocks-publication.

[jessegreenberg]

Even though #1626 (comment) is unlikely, I think we should avoid using FluentConstant and FluentPattern outside of the a11y key. That will make sure that #1626 (comment) is impossible to encounter. These problems will be something to work on when we support translation of these patterns.

[samreid]

Would it be safer to avoid pruning common code dependency strings, and just let rosetta decide which to show?

[jessegreenberg]

I am not sure! But I think this is the band aid we need to fix the issue sim-side.

Details

Subject: [PATCH] Add isLocaleChanging so that we can opt out of work while many LocalizedStringProperties change at once, see https://github.com/phetsims/chipper/issues/1611
---
Index: js/grunt/modulify/generateFluentTypes.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/modulify/generateFluentTypes.ts b/js/grunt/modulify/generateFluentTypes.ts
--- a/js/grunt/modulify/generateFluentTypes.ts	(revision f87199dcae4abd0c76a23f57b8df79cf9f027dee)
+++ b/js/grunt/modulify/generateFluentTypes.ts	(date 1756420690131)
@@ -309,7 +309,19 @@
 
       const accessor = createAccessor( [ ...pathArr, key ] );
       const getter = `_.get( ${pascalCaseRepo}Strings, '${accessor}' )`;
-      if ( ChipperStringUtils.isLegacyStringPattern( val ) ) {
+
+      // Do not pass the following strings through Fluent:
+      //
+      // - Strings that live outside the `a11y` namespace. They are not currently
+      //   translated; treating them as Fluent may introduce runtime errors if a
+      //   translator inserts Fluent syntax or references another key.
+      //
+      // - Strings that use legacy placeholder patterns such as `{0}` or `{{value}}`.
+      //   These are formatted at runtime with `StringUtils.format` or `StringUtils.fillIn`
+      //   and are therefore incompatible with FluentPattern.
+      //
+      // TODO: https://github.com/phetsims/rosetta/issues/221, Revisit when full Fluent support is available for non-a11y content.
+      if ( ChipperStringUtils.isLegacyStringPattern( val ) || !accessor.startsWith( 'a11y.' ) ) {
 
         // This is a legacy string and is meant to be used with StringUtils.format or
         // StringUtils.fillIn. It should use the LocalizedStringProperty directly.

[jessegreenberg]

@samreid and I met to discuss. We had several questions about how rosetta and the build packages strings. But we thought that committing #1626 (comment) would give us safety for now.

To see how the patch helps us I did the following test:

• Add someUnusedWithUndefinedReference: '{undefinedVariable}' to membrane transport yaml.
• Commented out this line in FluentLibrary: // throw new Error( `These messages are not defined: [ ${undefinedMessagesFormatted} ]` );
• Run grunt modulify.

This produced the following error when running the sim:

Assertion failed: Fluent errors found when formatting message: ReferenceError: Unknown message: undefinedVariable

After applying the patch in #1626 (comment) the problem went away. I think this demonstrates how a translator could break the sim.

@jonathanolson @samreid does this unblock publications? Should we maintenance release any simulations? The long term approach should be to find a solution for #1626 (comment). What else do we need for the short-medium term?