refactor(fbt): Improve FbtResult/logging separation of concerns

Summary:
We need to start eliminating these no-op mocks (which aren't customizable) and enable other callers to hook into to the same events/logic we have.  Here are the beginnings of that hooking mechanism.

Relevant issues (not yet fixed here):
#111
#59

Reviewed By: kayhadrin

Differential Revision: D18950531

fbshipit-source-id: 9ffd85d5659d40eb1f2f1a52174016bdfb8819af

.flowconfig

@@ -30,12 +30,12 @@ module.name_mapper='^\(invariant\|graceful-fs\|optimist\|fb-babel-plugin-utils\|
 module.name_mapper='^\(ReactDOM\)$' -> '<PROJECT_ROOT>/node_modules/react-dom'
 
 # Redirect non-FB JS modules to a dedicated folder
-module.name_mapper='^\(FBLocaleToLang\|fbtInit\|FbtPublic\|FbtTranslations\|GenderConst\|InlineFbtResult\|intlInlineMode\|IntlNumberType\|IntlPhonologicalRewrites\|IntlVariationResolver\|IntlVariations\|NumberFormatConsts\)$' -> '<PROJECT_ROOT>/runtime/nonfb/\1'
+module.name_mapper='^\(FBLocaleToLang\|fbtInit\|FbtEnv\|FbtPublic\|FbtTranslations\|GenderConst\|InlineFbtResult\|intlInlineMode\|IntlNumberType\|IntlPhonologicalRewrites\|IntlVariationResolver\|IntlVariations\|NumberFormatConsts\)$' -> '<PROJECT_ROOT>/runtime/nonfb/\1'
 module.name_mapper='^\(Banzai\|cx\|FBLogger\|FBLogMessage\|FbtLogger\|FbtQTOverrides\|FbtResultGK\|IntlViewerContext\|killswitch\)$' -> '<PROJECT_ROOT>/runtime/nonfb/mocks/\1'
 module.name_mapper='^\(DisplayGenderConst\)$' -> '<PROJECT_ROOT>/runtime/shared_deps/\1'
 
 # Redirect public JS modules to a dedicated folder
-module.name_mapper='^\(escapeRegex\|fbs\|fbt\|FbtPureStringResult\|FbtReactUtil\|FbtResultBaseImpl\|FbtResultBase\|FbtResult\|FbtResultWWW\|FbtTableAccessor\|formatNumber\|IntlGender\|intlList\|intlNumUtils\|IntlPunctuation\|IntlVariationResolverImpl\|substituteTokens\)$' -> '<PROJECT_ROOT>/runtime/shared/\1'
+module.name_mapper='^\(escapeRegex\|fbs\|fbt\|FbtHooks\|FbtPureStringResult\|FbtReactUtil\|FbtResultBaseImpl\|FbtResultBase\|FbtResult\|FbtResultWWW\|FbtTableAccessor\|formatNumber\|IntlGender\|intlList\|intlNumUtils\|IntlPunctuation\|IntlVariationResolverImpl\|substituteTokens\)$' -> '<PROJECT_ROOT>/runtime/shared/\1'
 module.name_mapper='^\(IntlCLDRNumberType[0-9][0-9]\)$' -> '<PROJECT_ROOT>/runtime/shared/FbtNumber/\1'
 # This Jest mock file is only used for testing, we don't need it on the OSS side
 module.name_mapper='^\(FbtNumberType\)$' -> '<PROJECT_ROOT>/runtime/shared/__mocks__/\1'

CHANGELOG.md

@@ -32,7 +32,8 @@ We haven't had the best track record of code/feature changes before this date, b
      Unreleased changes that have landed in master. Click to see more.
     </summary>
 
-- [refactor] Move FbtContentItem and NestedFbtContentItems to libdefs
+- [refactor] Abstract away string serialization error handling.
+- [refactor] Move FbtContentItem and $NestedFbtContentItems to libdefs
 - [refactor] Flow type strict substituteTokens.js
 - Fix version in header comments
 

flow-types/libdef/fbt.js

@@ -90,8 +90,46 @@ declare type $NestedFbtContentItems = $ReadOnlyArray<
   $FbtContentItem | $NestedFbtContentItems,
 >;
 
+declare type FbtErrorContext = {hash: ?string, translation: string};
+
+/**
+ * A delegate used in FbtResult for handling errors when toString
+ * can't serialize due to non-string and non-Fbt elements in the
+ * interpolated payload (e.g. React nodes, DOM nodes, etc).
+ */
+declare interface IFbtErrorListener {
+  constructor(context: FbtErrorContext): void;
+
+  /**
+   * Handle the error scenario where the FbtResultBase contains non-string elements
+   * (usually React components) and tries to run .toString()
+   *
+   * Example of bad usage of <fbt> with rich contents that will trigger this error
+   *
+   * render() {
+   *   const text = (
+   *     <fbt desc="...">
+   *       I have <Link href="#">no name</Link>.
+   *     </fbt>
+   *   );
+   *   return (
+   *     <div className={cx('FiddleCSS/root')}>
+   *       <p>Text = "{text}"</p>
+   *       <p>Truncated Text = "{text.substr(0, 9)}"</p> // will output: "I have ."
+   *       <em>You might have expected "I have no name" but we don't support
+   *           this in the FBT API.</em>
+   *     </div>
+   *   );
+   * }
+   */
+  onStringSerializationError(content: $FbtContentItem): void;
+}
+
 declare interface IFbtResultBase {
-  constructor(contents: $ReadOnlyArray<any>): void;
+  constructor(
+    contents: $ReadOnlyArray<any>,
+    errorListener: ?FbtErrorListener,
+  ): void;
   getContents(): any;
   // This relies on toString() which contains i18n logging logic to track impressions.
   // I.e. If you use this, i18n will register the string as displayed!
@@ -163,7 +201,10 @@ declare interface IFbtStringish {
 // - it doesn't behave like a stringish object
 declare class FbtPureStringResult implements IFbtResultBase {
   // implements IFbtResultBase
-  constructor(contents: $ReadOnlyArray<any>): void;
+  constructor(
+    contents: $ReadOnlyArray<any>,
+    errorListener: ?FbtErrorListener,
+  ): void;
   getContents: $PropertyType<IFbtResultBase, 'getContents'>;
   toJSON: $PropertyType<IFbtResultBase, 'toJSON'>;
   // TODO(T27672828) Move code of toString() inside unwrap()

runtime/nonfb/FbtEnv.js

@@ -0,0 +1,16 @@
+/**
+ * Copyright 2004-present Facebook. All Rights Reserved.
+ *
+ * @emails oncall+internationalization
+ * @flow strict
+ * @format
+ */
+
+/**
+ * An empty no-op for now.
+ */
+const FbtEnv = {
+  setup() {},
+};
+
+module.exports = FbtEnv;

runtime/nonfb/fbtInit.js

@@ -7,16 +7,24 @@
  * @format
  */
 
+const FbtHooks = require('FbtHooks');
 const FbtTranslations = require('FbtTranslations');
 
-function fbtInit({translations, customTranslationPayloadGetter__EXPERIMENTAL}) {
+function fbtInit({
+  customTranslationPayloadGetter__EXPERIMENTAL,
+  hooks,
+  translations,
+}) {
   FbtTranslations.registerTranslations(translations);
 
   if (customTranslationPayloadGetter__EXPERIMENTAL != null) {
     FbtTranslations.setCustomTranslationPayloadGetter__EXPERIMENTAL(
       customTranslationPayloadGetter__EXPERIMENTAL,
     );
   }
+  if (hooks != null) {
+    FbtHooks.register(hooks);
+  }
 }
 
 module.exports = fbtInit;

runtime/shared/FbtHooks.js

@@ -0,0 +1,27 @@
+/**
+ * Copyright 2004-present Facebook. All Rights Reserved.
+ *
+ * @emails oncall+internationalization
+ * @flow strict-local
+ * @format
+ */
+
+// import typeof FbtErrorContext from 'FbtErrorContext';
+
+export type FbtHookRegistrations = {
+  errorListener?: (context: FbtErrorContext) => IFbtErrorListener,
+};
+
+const _registrations: FbtHookRegistrations = {};
+
+const FbtHooks = {
+  register(registrations: FbtHookRegistrations): void {
+    Object.assign(_registrations, registrations);
+  },
+
+  getErrorListener(context: FbtErrorContext): ?IFbtErrorListener {
+    const factory = _registrations.errorListener;
+    return factory ? factory(context) : null;
+  },
+};
+module.exports = FbtHooks;

runtime/shared/FbtResult.js

@@ -31,8 +31,11 @@ class FbtResult extends FbtResultBaseImpl {
   ref: ?React$Ref<React$ElementType> = null;
   type: (props: Props) => mixed = FbtComponent;
 
-  constructor(contents: $NestedFbtContentItems) {
-    super(contents);
+  constructor(
+    contents: $NestedFbtContentItems,
+    errorListener: ?IFbtErrorListener,
+  ) {
+    super(contents, errorListener);
     /* eslint-disable fb-www/react-state-props-mutation */
     this.props = {
       content: contents,

runtime/shared/FbtResultBase.js

@@ -23,6 +23,7 @@ let hasImplementedStringishMethods = false;
 // Named _FbtResultBase to avoid colliding with `FbtResultBase` class definition in Flow
 class _FbtResultBase implements IFbtResultBase {
   _contents: $NestedFbtContentItems;
+  _errorListener: ?IFbtErrorListener;
   _stringValue: ?string;
 
   // Declare that we'll implement these methods
@@ -66,12 +67,16 @@ class _FbtResultBase implements IFbtResultBase {
   trimLeft: $PropertyType<IFbtResultBase, 'trimLeft'>;
   trimRight: $PropertyType<IFbtResultBase, 'trimRight'>;
 
-  constructor(contents: $NestedFbtContentItems) {
+  constructor(
+    contents: $NestedFbtContentItems,
+    errorListener: ?IFbtErrorListener,
+  ) {
     invariant(
       hasImplementedStringishMethods,
       'Stringish methods must be implemented. See `usingStringProxyMethod`.',
     );
     this._contents = contents;
+    this._errorListener = errorListener;
     this._stringValue = null;
   }
 
@@ -83,31 +88,6 @@ class _FbtResultBase implements IFbtResultBase {
     return this._contents;
   }
 
-  /**
-   * Handle the error scenario where the FbtResultBase contains non-string elements
-   * (usually React components) and tries to run .toString()
-   *
-   * Example of bad usage of <fbt> with rich contents that will trigger this error
-   *
-   * render() {
-   *   const text = (
-   *     <fbt desc="...">
-   *       I have <Link href="#">no name</Link>.
-   *     </fbt>
-   *   );
-   *   return (
-   *     <div className={cx('FiddleCSS/root')}>
-   *       <p>Text = "{text}"</p>
-   *       <p>Truncated Text = "{text.substr(0, 9)}"</p> // will output: "I have ."
-   *       <em>You might have expected "I have no" but we don't support this in the FBT API.</em>
-   *     </div>
-   *   );
-   * }
-   */
-  onStringSerializationError(_content: $FbtContentItem): void {
-    throw new Error('This method needs to be overridden by a child class');
-  }
-
   toString(): string {
     if (this._stringValue != null) {
       return this._stringValue;
@@ -118,8 +98,8 @@ class _FbtResultBase implements IFbtResultBase {
       const content = contents[ii];
       if (typeof content === 'string' || content instanceof _FbtResultBase) {
         stringValue += content.toString();
-      } else {
-        this.onStringSerializationError(content);
+      } else if (this._errorListener != null) {
+        this._errorListener.onStringSerializationError(content);
       }
     }
     if (!Object.isFrozen(this)) {

runtime/shared/FbtResultWWW.js

@@ -11,8 +11,6 @@
 const FBLogger = require('FBLogger');
 const FbtResultBase = require('FbtResultBase');
 
-const killswitch = require('killswitch');
-
 function logErrorUseStringMethod(methodName: string): void {
   // If the contents is array of length greater than one, then use the string
   // method will cause error
@@ -29,27 +27,7 @@ function logErrorUseStringMethod(methodName: string): void {
 /**
  * The FbtResultBase "implemented" module for www.
  */
-class FbtResultWWW extends FbtResultBase {
-  onStringSerializationError(content: $FbtContentItem): void {
-    let details = 'Context not logged.';
-    if (!killswitch('JS_RELIABILITY_FBT_LOGGING')) {
-      try {
-        const contentStr = JSON.stringify(content);
-        if (contentStr != null) {
-          details = contentStr.substr(0, 250);
-        }
-      } catch (err) {
-        // Catching circular reference error
-        details = err.message;
-      }
-    }
-    FBLogger('fbt')
-      .blameToPreviousDirectory()
-      .mustfix('Converting to a string will drop content data. %s', details);
-  }
-}
-
-const FbtResultWWWWithStringishMethods: Class<FbtResultBase> = FbtResultWWW.usingStringProxyMethod(
+const FbtResultWWWWithStringishMethods: Class<FbtResultBase> = FbtResultBase.usingStringProxyMethod(
   (methodName: $Keys<IFbtStringish>) => {
     return function() {
       logErrorUseStringMethod(methodName);

runtime/shared/__tests__/FbtResult-WWW-only-test.js

@@ -8,6 +8,7 @@
 jest.disableAutomock();
 console.error = jest.fn();
 
+const FbtErrorListenerWWW = require('FbtErrorListenerWWW');
 const FbtResult = require('FbtResult');
 const React = require('React');
 const ReactTestUtils = require('ReactTestUtils');
@@ -24,17 +25,18 @@ describe('FbtResult: WWW-only', function() {
 
   it('invokes onStringSerializationError() when being serialized with non-FBT contents', function() {
     const nonFbtContent = /non_fbt_content/;
-    const result = new FbtResult(['kombucha', nonFbtContent]);
-    result.onStringSerializationError = jest.fn(function() {
-      // spy on the original method, but preserve original behavior
-      return FbtResult.prototype.onStringSerializationError.apply(
+    const listener = new FbtErrorListenerWWW({translation: 'f4k3', hash: null});
+    // spy on the original method, but preserve original behavior
+    listener.onStringSerializationError = jest.fn(function() {
+      return FbtErrorListenerWWW.prototype.onStringSerializationError.apply(
         this,
         arguments,
       );
     });
+    const result = new FbtResult(['kombucha', nonFbtContent], listener);
     result.toString();
-    expect(result.onStringSerializationError).toHaveBeenCalledTimes(1);
-    expect(result.onStringSerializationError).toHaveBeenCalledWith(
+    expect(listener.onStringSerializationError).toHaveBeenCalledTimes(1);
+    expect(listener.onStringSerializationError).toHaveBeenCalledWith(
       nonFbtContent,
     );
   });

runtime/shared/fbs.js

@@ -6,7 +6,7 @@
  *
  * Wrapper module for fbt.js (the implementation)
  */
-
+const FbtHooks = require('FbtHooks');
 const FbtPureStringResult = require('FbtPureStringResult');
 
 const fbt = require('fbt');
@@ -31,9 +31,11 @@ const FbsImpl = {
     return fbt._param(...arguments);
   },
 
-  _wrapContent(fbtContent, patternString, patternHash) {
+  _wrapContent(fbtContent, translation, hash) {
     const contents = typeof fbtContent === 'string' ? [fbtContent] : fbtContent;
-    return new FbtPureStringResult(contents);
+    // TODO T59083776: Pass in errorListener and update constructor arguments
+    const errorListener = FbtHooks.getErrorListener({hash, translation});
+    return new FbtPureStringResult(contents, errorListener);
   },
 };
 

runtime/shared/fbt.js

@@ -23,10 +23,12 @@
 /* eslint "fb-www/avoid-this-outside-classes": "off" */
 /* eslint "fb-www/order-requires": "off" */
 /* eslint "fb-www/require-flow-strict-local": "off" */
+require('FbtEnv').setup();
 
 const Banzai = require('Banzai');
 const {logger} = require('FbtLogger');
 const {overrides} = require('FbtQTOverrides');
+const FbtHooks = require('FbtHooks');
 const FbtResultBase = require('FbtResultBase');
 const FbtTableAccessor = require('FbtTableAccessor');
 const FbtResult = require('FbtResult');
@@ -44,8 +46,7 @@ const {
   getGenderVariations,
 } = require('IntlVariationResolver');
 
-// Used only in React Native
-let jsonExportMode = false;
+let jsonExportMode = false; // Used only in React Native
 
 const BINAST_STRING_MAGIC_CODE = 'B!N@$T';
 /**
@@ -493,7 +494,11 @@ fbt._wrapContent = (fbtContent, patternString, patternHash) => {
       patternHash,
     );
   }
-  return new FbtResult(contents);
+  const errorListener = FbtHooks.getErrorListener({
+    translation: patternString,
+    hash: patternHash,
+  });
+  return new FbtResult(contents, errorListener);
 };
 
 fbt.enableJsonExportMode = function() {