[@fluent/react] Provided better errors when components are mis-configured (#549)

gregtatum · web-flow · commit 572cbbc86e7a · 2021-04-19T10:34:31.000-05:00
* Throw an error when the LocalizationProvider is not properly initialized

The current behavior is permissive, and works without warnings. This
change makes it so that omitting the LocalizationProvider throws an
error. I was originally trying to decide between throwing an error and
emitting a console error. I opted for an error to have the behavior in
line with what the react-redux Provider does.

This is potentially a breaking change for users that are relying on the
LocalizationProvider being omitted before the bundles are loaded. This
behavior can still be achieved by providing an empty bundle list to the
LocalizationProvider.

* Report an error when there is no matching message id

* Fix lint issues
diff --git a/fluent-react/src/.eslintrc.js b/fluent-react/src/.eslintrc.js
@@ -0,0 +1,3 @@
+module.exports = {
+  extends: "../../eslint_ts.json"
+}
diff --git a/fluent-react/src/context.ts b/fluent-react/src/context.ts
@@ -1,4 +1,5 @@
 import { createContext } from "react";
 import { ReactLocalization } from "./localization";
 
-export let FluentContext = createContext(new ReactLocalization([], null));
+export let FluentContext =
+  createContext(null) as React.Context<ReactLocalization | null>;
diff --git a/fluent-react/src/localization.ts b/fluent-react/src/localization.ts
@@ -30,6 +30,12 @@ export class ReactLocalization {
     return mapBundleSync(this.bundles, id);
   }
 
+  areBundlesEmpty(): boolean {
+    // Create an iterator and only peek at the first value to see if it contains
+    // anything.
+    return Boolean(this.bundles[Symbol.iterator]().next().done);
+  }
+
   getString(
     id: string,
     args?: Record<string, FluentVariable> | null,
@@ -46,6 +52,22 @@ export class ReactLocalization {
         }
         return value;
       }
+    } else {
+      if (this.areBundlesEmpty()) {
+        this.reportError(
+          new Error(
+            "Attempting to get a string when no localization bundles are " +
+              "present."
+          )
+        );
+      } else {
+        this.reportError(
+          new Error(
+            `The id "${id}" did not match any messages in the localization `
+              + "bundles."
+          )
+        );
+      }
     }
 
     return fallback || id;
diff --git a/fluent-react/src/localized.ts b/fluent-react/src/localized.ts
@@ -66,13 +66,36 @@ export function Localized(props: LocalizedProps): ReactElement {
   }
 
   if (!l10n) {
-    // Use the wrapped component as fallback.
-    return createElement(Fragment, null, child);
+    throw new Error(
+      "The <Localized /> component was not properly wrapped in a "
+        + "<LocalizationProvider />."
+    );
   }
 
   const bundle = l10n.getBundle(id);
 
   if (bundle === null) {
+    if (id === undefined) {
+      l10n.reportError(
+        new Error("No id was provided for a <Localized /> component.")
+      );
+    } else {
+      if (l10n.areBundlesEmpty()) {
+        l10n.reportError(
+          new Error(
+            "A <Localized /> component was rendered when no localization "
+              + "bundles are present."
+          )
+        );
+      } else {
+        l10n.reportError(
+          new Error(
+            `The id "${id}" did not match any messages in the localization `
+              + "bundles."
+          )
+        );
+      }
+    }
     // Use the wrapped component as fallback.
     return createElement(Fragment, null, child);
   }
diff --git a/fluent-react/src/use_localization.ts b/fluent-react/src/use_localization.ts
@@ -9,5 +9,12 @@ type useLocalization = () => { l10n: ReactLocalization }
 export const useLocalization: useLocalization = () => {
   const l10n = useContext(FluentContext);
 
+  if (!l10n) {
+    throw new Error(
+      "useLocalization was used without wrapping it in a "
+        + "<LocalizationProvider />."
+    );
+  }
+
   return { l10n };
 };
diff --git a/fluent-react/src/with_localization.ts b/fluent-react/src/with_localization.ts
@@ -17,6 +17,12 @@ export function withLocalization<P extends WithLocalizationProps>(
 ): ComponentType<WithoutLocalizationProps<P>> {
   function WithLocalization(props: WithoutLocalizationProps<P>): ReactElement {
     const l10n = useContext(FluentContext);
+    if (!l10n) {
+      throw new Error(
+        "withLocalization was used without wrapping it in a "
+          + "<LocalizationProvider />."
+      );
+    }
     // Re-bind getString to trigger a re-render of Inner.
     const getString = l10n.getString.bind(l10n);
     return createElement(Inner, { getString, ...props } as P);
diff --git a/fluent-react/test/.eslintrc.js b/fluent-react/test/.eslintrc.js
@@ -0,0 +1,3 @@
+module.exports = {
+  extends: "../../eslint_test.json"
+}
diff --git a/fluent-react/test/localized_fallback.test.js b/fluent-react/test/localized_fallback.test.js
@@ -61,6 +61,8 @@ foo = FOO
 });
 
 test("falls back back for missing message", function() {
+  jest.spyOn(console, "warn").mockImplementation(() => {});
+
   const bundle1 = new FluentBundle();
   bundle1.addResource(
     new FluentResource(`
@@ -81,4 +83,12 @@ not-foo = NOT FOO
       Bar
     </div>
   `);
+
+  expect(console.warn.mock.calls).toMatchInlineSnapshot(`
+    Array [
+      Array [
+        "[@fluent/react] Error: The id \\"foo\\" did not match any messages in the localization bundles.",
+      ],
+    ]
+  `);
 });
diff --git a/fluent-react/test/localized_render.test.js b/fluent-react/test/localized_render.test.js
@@ -336,11 +336,11 @@ foo = { $arg }
   });
 
   test("render with a fragment and no message preserves the fragment", () => {
-    const bundle = new FluentBundle();
+    jest.spyOn(console, "warn").mockImplementation(() => {});
 
     const renderer = TestRenderer.create(
-      <LocalizationProvider l10n={new ReactLocalization([bundle])}>
-        <Localized id="foo">
+      <LocalizationProvider l10n={new ReactLocalization([])}>
+        <Localized id="non-matching-id">
           <React.Fragment>
             <div>Fragment content</div>
           </React.Fragment>
@@ -349,10 +349,11 @@ foo = { $arg }
     );
 
     expect(renderer.toJSON()).toMatchInlineSnapshot(`
-                  <div>
-                    Fragment content
-                  </div>
-            `);
+      <div>
+        Fragment content
+      </div>
+    `);
+    expect(console.warn).toHaveBeenCalled();
   });
 
   test("A missing $arg does not break rendering", () => {
@@ -442,6 +443,7 @@ foo = Test message
 
   test("render with an empty fragment and no message preserves the fragment", () => {
     const bundle = new FluentBundle();
+    jest.spyOn(console, "warn").mockImplementation(() => {});
 
     const renderer = TestRenderer.create(
       <LocalizationProvider l10n={new ReactLocalization([bundle])}>
@@ -452,6 +454,13 @@ foo = Test message
     );
 
     expect(renderer.toJSON()).toMatchInlineSnapshot(`null`);
+    expect(console.warn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "[@fluent/react] Error: The id \\"foo\\" did not match any messages in the localization bundles.",
+        ],
+      ]
+    `);
   });
 
   test("render with an empty fragment and no message value preserves the fragment", () => {
@@ -494,6 +503,7 @@ foo = Test message
   });
 
   test("render with a string fallback and no message returns the fallback", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
     const bundle = new FluentBundle();
 
     const renderer = TestRenderer.create(
@@ -503,6 +513,13 @@ foo = Test message
     );
 
     expect(renderer.toJSON()).toMatchInlineSnapshot(`"String fallback"`);
+    expect(console.warn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "[@fluent/react] Error: The id \\"foo\\" did not match any messages in the localization bundles.",
+        ],
+      ]
+    `);
   });
 
   test("render with a string fallback returns the message", () => {
@@ -541,6 +558,7 @@ foo = Message
   });
 
   test("render without a fallback and no message returns nothing", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
     const bundle = new FluentBundle();
 
     const renderer = TestRenderer.create(
@@ -550,5 +568,12 @@ foo = Message
     );
 
     expect(renderer.toJSON()).toMatchInlineSnapshot(`null`);
+    expect(console.warn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "[@fluent/react] Error: The id \\"foo\\" did not match any messages in the localization bundles.",
+        ],
+      ]
+    `);
   });
 });
diff --git a/fluent-react/test/localized_valid.test.js b/fluent-react/test/localized_valid.test.js
@@ -5,48 +5,70 @@ import {
   LocalizationProvider,
   Localized
 } from "../esm/index";
+import { FluentBundle, FluentResource } from "@fluent/bundle";
 
 describe("Localized - validation", () => {
-  test("inside of a LocalizationProvider", () => {
+  function createValidBundle() {
+    const validBundle = new FluentBundle();
+    validBundle.addResource(
+      new FluentResource("example-id = Example localized text\n")
+    );
+    return validBundle;
+  }
+
+  test("renders with no errors or warnings when correctly created inside of a LocalizationProvider", () => {
     const renderer = TestRenderer.create(
-      <LocalizationProvider l10n={new ReactLocalization([])}>
-        <Localized>
+      <LocalizationProvider l10n={new ReactLocalization([createValidBundle()])}>
+        <Localized id="example-id">
           <div />
         </Localized>
       </LocalizationProvider>
     );
 
-    expect(renderer.toJSON()).toMatchInlineSnapshot(`<div />`);
+    expect(renderer.toJSON()).toMatchInlineSnapshot(`
+      <div>
+        Example localized text
+      </div>
+    `);
   });
 
-  test("outside of a LocalizationProvider", () => {
-    const renderer = TestRenderer.create(
-      <Localized>
-        <div />
-      </Localized>
+  test("throws an error when placed outside of a LocalizationProvider", () => {
+    jest.spyOn(console, "error").mockImplementation(() => {});
+
+    expect(() => {
+      TestRenderer.create(
+        <Localized id="example-id">
+          <div />
+        </Localized>
+      );
+    }).toThrowErrorMatchingInlineSnapshot(
+      `"The <Localized /> component was not properly wrapped in a <LocalizationProvider />."`
     );
 
-    expect(renderer.toJSON()).toMatchInlineSnapshot(`<div />`);
+    // React also does a console.error.
+    expect(console.error).toHaveBeenCalled();
   });
 
-  test("without a child", () => {
+  test("renders the localized text when no child is provided", () => {
     const renderer = TestRenderer.create(
-      <LocalizationProvider l10n={new ReactLocalization([])}>
-        <Localized />
+      <LocalizationProvider l10n={new ReactLocalization([createValidBundle()])}>
+        <Localized id="example-id" />
       </LocalizationProvider>
     );
 
-    expect(renderer.toJSON()).toMatchInlineSnapshot(`null`);
+    expect(renderer.toJSON()).toMatchInlineSnapshot(`"Example localized text"`);
   });
 
-  test("with multiple children", () => {
+  test("throws when multiple children are provided", () => {
     // React also does a console.error, ignore that here.
     jest.spyOn(console, "error").mockImplementation(() => {});
 
     expect(() => {
       TestRenderer.create(
-        <LocalizationProvider l10n={new ReactLocalization([])}>
-          <Localized>
+        <LocalizationProvider
+          l10n={new ReactLocalization([createValidBundle()])}
+        >
+          <Localized id="example-id">
             <div />
             <div />
           </Localized>
@@ -57,17 +79,23 @@ describe("Localized - validation", () => {
     expect(console.error).toHaveBeenCalled();
   });
 
-  test("with single children inside an array", () => {
+  test("is valid to have a children array with a single element inside", () => {
     const renderer = TestRenderer.create(
-      <LocalizationProvider l10n={new ReactLocalization([])}>
-        <Localized children={[<div />]} />
+      <LocalizationProvider l10n={new ReactLocalization([createValidBundle()])}>
+        <Localized id="example-id" children={[<div />]} />
       </LocalizationProvider>
     );
 
-    expect(renderer.toJSON()).toMatchInlineSnapshot(`<div />`);
+    expect(renderer.toJSON()).toMatchInlineSnapshot(`
+      <div>
+        Example localized text
+      </div>
+    `);
   });
 
-  test("without id", () => {
+  test("has a warning when no id is provided", () => {
+    jest.spyOn(console, "warn").mockImplementation(() => {});
+
     const renderer = TestRenderer.create(
       <LocalizationProvider l10n={new ReactLocalization([])}>
         <Localized>
@@ -77,5 +105,13 @@ describe("Localized - validation", () => {
     );
 
     expect(renderer.toJSON()).toMatchInlineSnapshot(`<div />`);
+
+    expect(console.warn.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "[@fluent/react] Error: No id was provided for a <Localized /> component.",
+        ],
+      ]
+    `);
   });
 });
diff --git a/fluent-react/test/use_localization.test.js b/fluent-react/test/use_localization.test.js
diff --git a/fluent-react/test/with_localization.test.js b/fluent-react/test/with_localization.test.js

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ extends: "../../eslint_ts.json"`
	`3`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ extends: "../../eslint_test.json"`
	`3`	`+}`