feat(fbt): prevent using duplicate token substitutions

Reviewed By: pkqinys

Differential Revision: D33476399

fbshipit-source-id: 8b1bcae962955474529457c6e520dd5742d3b6c3

CHANGELOG.md

@@ -58,6 +58,7 @@ List of changes for each released npm package version.
     Unreleased changes that have landed in main. Click to see more.
   </summary>
 
+  - [feat] Add sanity check to ensure that each patter string token is mapped to a single substitution value.
   - [chore] Add `.npmignore` config (avoid exporting some build, debug & test files)
 
 </details>

flow-types/libdef/fbt.js

@@ -130,6 +130,8 @@ declare interface IFbtErrorListener {
    */
   +onStringSerializationError?: (content: $FbtContentItem) => void;
 
+  +onDuplicateSubstitutionTokenError?: (duplicateTokenName: string) => void;
+
   +onStringMethodUsed?: (method: string) => void;
 }
 
@@ -241,11 +243,8 @@ type $GenericFbtFunctionAPI<Input, Output, ParamInput, ParamOutput> = {
   ...
 };
 
-type $StringBasedFbtFunctionAPI<
-  Output,
-  ParamInput,
-  ParamOutput,
-> = $GenericFbtFunctionAPI<string, Output, ParamInput, ParamOutput>;
+type $StringBasedFbtFunctionAPI<Output, ParamInput, ParamOutput> =
+  $GenericFbtFunctionAPI<string, Output, ParamInput, ParamOutput>;
 
 /**
  * NOTE how the fbs() functional API relies on using an array of content items

runtime/shared/__tests__/fbt-test.js

@@ -11,12 +11,8 @@
 
 'use strict';
 
-import type {IntlVariationsEnum} from 'IntlVariations';
-
-jest.mock('FbtNumberType');
-jest.mock('translationOverrideListener'); // FB internal
-
 import type {FbtRuntimeCallInput, FbtTranslatedInput} from 'FbtHooks';
+import type {IntlVariationsEnum} from 'IntlVariations';
 
 const GenderConst = require('GenderConst');
 // Warning: importing JS modules outside of beforeEach blocks is generally bad practice
@@ -28,6 +24,9 @@ const ReactDOM = require('ReactDOMLegacy_DEPRECATED');
 const invariant = require('invariant');
 const React = require('react');
 
+jest.mock('FbtNumberType');
+jest.mock('translationOverrideListener'); // FB internal
+
 let domContainer;
 let fbt;
 let fbtRuntime;
@@ -229,6 +228,49 @@ describe('fbt', () => {
   //   );
   // });
 
+  describe('when encountering duplicate token substitutions', () => {
+    let FbtFBLogger;
+    beforeEach(() => {
+      jest.mock('FBLogger', () =>
+        jest.fn(_loggerProject => ({
+          blameToPreviousDirectory: jest.fn(() => ({
+            blameToPreviousDirectory: jest.fn(() => {
+              return (FbtFBLogger = {
+                mustfix: jest.fn(() => {}),
+              });
+            }),
+          })),
+        })),
+      );
+    });
+
+    it('should log the duplicate token coming from the same type of construct', () => {
+      fbtRuntime._('Just a {tokenName}', [
+        fbtRuntime._param('tokenName', 'substitute'),
+        fbtRuntime._param('tokenName', 'substitute'),
+      ]);
+      expect(FbtFBLogger.mustfix).toHaveBeenCalledWith(
+        'Cannot register a substitution with token=`%s` more than once',
+        'tokenName',
+      );
+    });
+
+    it('should log the duplicate token coming from the different constructs', () => {
+      fbtRuntime._('Just a {tokenName}', [
+        fbtRuntime._param('tokenName', 'substitute'),
+        fbtRuntime._name(
+          'tokenName',
+          'person name',
+          IntlVariations.GENDER_UNKNOWN,
+        ),
+      ]);
+      expect(FbtFBLogger.mustfix).toHaveBeenCalledWith(
+        'Cannot register a substitution with token=`%s` more than once',
+        'tokenName',
+      );
+    });
+  });
+
   it('should replace QuickTranslation strings', function () {
     expect(
       fbtRuntime._(['This is a QT string', '8b0c31a270a324f26d2417a358106611']),
@@ -337,7 +379,7 @@ describe('fbt', () => {
   });
 
   describe(': given a string with implicit parameters', () => {
-    function getFbt({viewer, ownerGender, object, count}) {
+    function getFbt({count, object, ownerGender, viewer}) {
       return (
         <fbt desc="description">
           <fbt:name gender={viewer.gender} name="name">

runtime/shared/fbt.js

@@ -19,14 +19,17 @@
  * @emails oncall+i18n_fbt_js
  */
 
-require('FbtEnv').setupOnce();
+/* eslint-disable fb-www/order-requires */
 
 import type {FbtInputOpts, FbtRuntimeInput, FbtTableArgs} from 'FbtHooks';
 import type {ParamVariationType, ValidPronounUsagesType} from 'FbtRuntimeTypes';
 import type {FbtTableKey, PatternHash, PatternString} from 'FbtTable';
 import type {FbtTableArg} from 'FbtTableAccessor';
 import type {GenderConstEnum} from 'GenderConst';
 
+const FbtEnv = require('FbtEnv');
+FbtEnv.setupOnce();
+
 const FbtHooks = require('FbtHooks');
 const {overrides} = require('FbtQTOverrides');
 const FbtResultBase = require('FbtResultBase');
@@ -42,6 +45,12 @@ const intlNumUtils = require('intlNumUtils');
 const invariant = require('invariant');
 const substituteTokens = require('substituteTokens');
 
+/*
+ * $FlowFixMe[method-unbinding] Use original method in case the token names contain
+ * a 'hasOwnProperty' key too; or if userland code redefined that method.
+ */
+const {hasOwnProperty} = Object.prototype;
+
 let jsonExportMode = false; // Used only in React Native
 
 const {ARG} = FbtTable;
@@ -145,10 +154,8 @@ function fbtCallsite(
       // translations. If pattern is not a string here, table has not been accessed
       pattern = FbtTable.access(pattern, args, 0);
     }
-    allSubstitutions = Object.assign(
-      {},
-      ...args.map(arg => arg[ARG.SUBSTITUTION] || {}),
-    );
+
+    allSubstitutions = getAllSubstitutions(args);
     invariant(pattern !== null, 'Table access failed');
   }
 
@@ -198,6 +205,28 @@ function fbtCallsite(
   }
 }
 
+function getAllSubstitutions(args) {
+  const allSubstitutions = {};
+  args.forEach(arg => {
+    const substitution = arg[ARG.SUBSTITUTION];
+    if (!substitution) {
+      return;
+    }
+    for (const tokenName in substitution) {
+      if (hasOwnProperty.call(substitution, tokenName)) {
+        if (allSubstitutions[tokenName] != null) {
+          FbtHooks.getErrorListener({
+            translation: '<<unresolved translation>>',
+            hash: null,
+          })?.onDuplicateSubstitutionTokenError?.(tokenName);
+        }
+        allSubstitutions[tokenName] = substitution[tokenName];
+      }
+    }
+  });
+  return allSubstitutions;
+}
+
 /**
  * _hasKeys takes an object and returns whether it has any keys. It purposefully
  * avoids creating the temporary arrays incurred by calling Object.keys(o)