diff --git a/.flowconfig b/.flowconfig index fff035fc..999bb00b 100644 --- a/.flowconfig +++ b/.flowconfig @@ -30,12 +30,12 @@ module.name_mapper='^\(invariant\|graceful-fs\|optimist\|fb-babel-plugin-utils\| module.name_mapper='^\(ReactDOM\)$' -> '/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\)$' -> '/runtime/nonfb/\1' +module.name_mapper='^\(FBLocaleToLang\|fbtInit\|FbtEnv\|FbtPublic\|FbtTranslations\|GenderConst\|InlineFbtResult\|intlInlineMode\|IntlNumberType\|IntlPhonologicalRewrites\|IntlVariationResolver\|IntlVariations\|NumberFormatConsts\)$' -> '/runtime/nonfb/\1' module.name_mapper='^\(Banzai\|cx\|FBLogger\|FBLogMessage\|FbtLogger\|FbtQTOverrides\|FbtResultGK\|IntlViewerContext\|killswitch\)$' -> '/runtime/nonfb/mocks/\1' module.name_mapper='^\(DisplayGenderConst\)$' -> '/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\)$' -> '/runtime/shared/\1' +module.name_mapper='^\(escapeRegex\|fbs\|fbt\|FbtHooks\|FbtPureStringResult\|FbtReactUtil\|FbtResultBaseImpl\|FbtResultBase\|FbtResult\|FbtResultWWW\|FbtTableAccessor\|formatNumber\|IntlGender\|intlList\|intlNumUtils\|IntlPunctuation\|IntlVariationResolverImpl\|substituteTokens\)$' -> '/runtime/shared/\1' module.name_mapper='^\(IntlCLDRNumberType[0-9][0-9]\)$' -> '/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\)$' -> '/runtime/shared/__mocks__/\1' diff --git a/CHANGELOG.md b/CHANGELOG.md index 49211441..d152ea53 100644 --- a/CHANGELOG.md +++ b/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. -- [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 diff --git a/flow-types/libdef/fbt.js b/flow-types/libdef/fbt.js index 7d51dfe3..de711341 100644 --- a/flow-types/libdef/fbt.js +++ b/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 with rich contents that will trigger this error + * + * render() { + * const text = ( + * + * I have no name. + * + * ); + * return ( + *
+ *

Text = "{text}"

+ *

Truncated Text = "{text.substr(0, 9)}"

// will output: "I have ." + * You might have expected "I have no name" but we don't support + * this in the FBT API. + *
+ * ); + * } + */ + onStringSerializationError(content: $FbtContentItem): void; +} + declare interface IFbtResultBase { - constructor(contents: $ReadOnlyArray): void; + constructor( + contents: $ReadOnlyArray, + 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): void; + constructor( + contents: $ReadOnlyArray, + errorListener: ?FbtErrorListener, + ): void; getContents: $PropertyType; toJSON: $PropertyType; // TODO(T27672828) Move code of toString() inside unwrap() diff --git a/runtime/nonfb/FbtEnv.js b/runtime/nonfb/FbtEnv.js new file mode 100644 index 00000000..62428667 --- /dev/null +++ b/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; diff --git a/runtime/nonfb/fbtInit.js b/runtime/nonfb/fbtInit.js index 9e69dccc..64cd8412 100644 --- a/runtime/nonfb/fbtInit.js +++ b/runtime/nonfb/fbtInit.js @@ -7,9 +7,14 @@ * @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) { @@ -17,6 +22,9 @@ function fbtInit({translations, customTranslationPayloadGetter__EXPERIMENTAL}) { customTranslationPayloadGetter__EXPERIMENTAL, ); } + if (hooks != null) { + FbtHooks.register(hooks); + } } module.exports = fbtInit; diff --git a/runtime/shared/FbtHooks.js b/runtime/shared/FbtHooks.js new file mode 100644 index 00000000..7ec51ac5 --- /dev/null +++ b/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; diff --git a/runtime/shared/FbtResult.js b/runtime/shared/FbtResult.js index d722c08e..24ea78d8 100644 --- a/runtime/shared/FbtResult.js +++ b/runtime/shared/FbtResult.js @@ -31,8 +31,11 @@ class FbtResult extends FbtResultBaseImpl { ref: ?React$Ref = 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, diff --git a/runtime/shared/FbtResultBase.js b/runtime/shared/FbtResultBase.js index 237ef91b..7410115f 100644 --- a/runtime/shared/FbtResultBase.js +++ b/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; trimRight: $PropertyType; - 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 with rich contents that will trigger this error - * - * render() { - * const text = ( - * - * I have no name. - * - * ); - * return ( - *
- *

Text = "{text}"

- *

Truncated Text = "{text.substr(0, 9)}"

// will output: "I have ." - * You might have expected "I have no" but we don't support this in the FBT API. - *
- * ); - * } - */ - 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)) { diff --git a/runtime/shared/FbtResultWWW.js b/runtime/shared/FbtResultWWW.js index f255a2bc..35e718c7 100644 --- a/runtime/shared/FbtResultWWW.js +++ b/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 = FbtResultWWW.usingStringProxyMethod( +const FbtResultWWWWithStringishMethods: Class = FbtResultBase.usingStringProxyMethod( (methodName: $Keys) => { return function() { logErrorUseStringMethod(methodName); diff --git a/runtime/shared/__tests__/FbtResult-WWW-only-test.js b/runtime/shared/__tests__/FbtResult-WWW-only-test.js index b6f38395..ae726e7e 100644 --- a/runtime/shared/__tests__/FbtResult-WWW-only-test.js +++ b/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, ); }); diff --git a/runtime/shared/fbs.js b/runtime/shared/fbs.js index 41fd0be5..139ca56f 100644 --- a/runtime/shared/fbs.js +++ b/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); }, }; diff --git a/runtime/shared/fbt.js b/runtime/shared/fbt.js index db5b732c..90008b01 100644 --- a/runtime/shared/fbt.js +++ b/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() {