fluent-react: Filter props with <Localized attrs={{…}}> (#141)

stasm · web-flow · commit ed3ac6472671 · 2018-02-01T21:03:00.000+01:00
The &lt;Localized&gt; component now requires the `attrs` prop to set any localized
attributes as props on the wrapped component. `attrs` should be an object with
attribute names as keys and booleans as values.

```jsx
&lt;Localized id="type-name" attrs={{placeholder: true}}&gt;
    &lt;input
        type="text"
        placeholder="Localizable placeholder"
        value={name}
        onChange={…}
    /&gt;
&lt;/Localized&gt;
```

By default, if `attrs` is not passed, no attributes will be set. This is
a breaking change compared to the previous behavior: in `fluent-react`
0.4.1 and before `&lt;Localized&gt;` would set _all_ attributes found in the
translation.
diff --git a/fluent-react/CHANGELOG.md b/fluent-react/CHANGELOG.md
@@ -50,8 +50,59 @@
     the element passed as a prop is cloned with the translated text content
     taken from the `DocumentFragment` used as `children`.
 
+  - Filter props set by translations with <Localized attrs={{…}}>.
+
+    The `<Localized>` component now requires the `attrs` prop to set any
+    localized attributes as props on the wrapped component. `attrs` should be
+    an object with attribute names as keys and booleans as values.
+
+    ```jsx
+    <Localized id="type-name" attrs={{placeholder: true}}>
+        <input
+            type="text"
+            placeholder="Localizable placeholder"
+            value={name}
+            onChange={…}
+        />
+    </Localized>
+    ```
+
+    By default, if `attrs` is not passed, no attributes will be set. This is
+    a breaking change compared to the previous behavior: in `fluent-react`
+    0.4.1 and before `<Localized>` would set _all_ attributes found in the
+    translation.
 
- #### Migrating from `fluent-react` 0.4.1
+#### Migrating from `fluent-react` 0.4.1 to 0.6.0
+
+If you're setting localized attributes as props of elements wrapped in
+`<Localized>`, in `fluent-react` 0.6.0 you'll need to also explicitly allow
+the props you're interested in using the `attrs` prop. This protects your
+components from accidentally gaining props they aren't expecting or from
+translations overwriting important props which shouldn't change.
+
+```jsx
+// BEFORE (fluent-react 0.4.1)
+<Localized id="type-name">
+    <input
+        type="text"
+        placeholder="Localizable placeholder"
+        value={name}
+        onChange={…}
+    />
+</Localized>
+```
+
+```jsx
+// AFTER (fluent-react 0.6.0)
+<Localized id="type-name" attrs={{placeholder: true}}>
+    <input
+        type="text"
+        placeholder="Localizable placeholder"
+        value={name}
+        onChange={…}
+    />
+</Localized>
+```
 
 In `fluent-react` 0.4.1 it was possible to pass React elements as _external
 arguments_ to localization via the `$`-prefixed props, just like you'd pass
diff --git a/fluent-react/examples/text-input/src/App.js b/fluent-react/examples/text-input/src/App.js
@@ -32,10 +32,10 @@ export default class App extends Component {
             </Localized>
         }
 
-        <Localized id="type-name">
+        <Localized id="type-name" attrs={{placeholder: true}}>
           <input
             type="text"
-            placeholder="Your name"
+            placeholder="Type your name"
             onChange={evt => this.handleNameChange(evt.target.value)}
             value={name}
           />
diff --git a/fluent-react/src/localized.js b/fluent-react/src/localized.js
@@ -76,7 +76,7 @@ export default class Localized extends Component {
 
   render() {
     const { l10n } = this.context;
-    const { id, children } = this.props;
+    const { id, attrs, children } = this.props;
     const elem = Children.only(children);
 
     if (!l10n) {
@@ -93,13 +93,29 @@ export default class Localized extends Component {
 
     const msg = mcx.getMessage(id);
     const [args, elems] = toArguments(this.props);
-    const { value, attrs } = l10n.formatCompound(mcx, msg, args);
+    const {
+      value: messageValue,
+      attrs: messageAttrs
+    } = l10n.formatCompound(mcx, msg, args);
+
+    // The default is to forbid all message attributes. If the attrs prop exists
+    // on the Localized instance, only set message attributes which have been
+    // explicitly allowed by the developer.
+    if (attrs && messageAttrs) {
+      var localizedProps = {};
+
+      for (const [name, value] of Object.entries(messageAttrs)) {
+        if (attrs[name]) {
+          localizedProps[name] = value;
+        }
+      }
+    }
 
-    if (value === null || !value.includes('<')) {
-      return cloneElement(elem, attrs, value);
+    if (messageValue === null || !messageValue.includes('<')) {
+      return cloneElement(elem, localizedProps, messageValue);
     }
 
-    const translationNodes = Array.from(parseMarkup(value).childNodes);
+    const translationNodes = Array.from(parseMarkup(messageValue).childNodes);
     const translatedChildren = translationNodes.map(childNode => {
       if (childNode.nodeType === childNode.TEXT_NODE) {
         return childNode.textContent;
@@ -122,7 +138,7 @@ export default class Localized extends Component {
       );
     });
 
-    return cloneElement(elem, attrs, ...translatedChildren);
+    return cloneElement(elem, localizedProps, ...translatedChildren);
   }
 }
 
diff --git a/fluent-react/test/localized_render_test.js b/fluent-react/test/localized_render_test.js
@@ -7,7 +7,7 @@ import ReactLocalization from '../src/localization';
 import { Localized } from '../src/index';
 
 suite('Localized - rendering', function() {
-  test('rendering the value', function() {
+  test('render the value', function() {
     const mcx = new MessageContext();
     const l10n = new ReactLocalization([mcx]);
 
@@ -27,7 +27,7 @@ foo = FOO
     ));
   });
 
-  test('rendering the attributes', function() {
+  test('render an allowed attribute', function() {
     const mcx = new MessageContext();
     const l10n = new ReactLocalization([mcx]);
 
@@ -37,7 +37,7 @@ foo
 `)
 
     const wrapper = shallow(
-      <Localized id="foo">
+      <Localized id="foo" attrs={{attr: true}}>
         <div />
       </Localized>,
       { context: { l10n } }
@@ -48,7 +48,50 @@ foo
     ));
   });
 
-  test('preserves existing attributes', function() {
+  test('only render allowed attributes', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .attr1 = ATTR 1
+    .attr2 = ATTR 2
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo" attrs={{attr2: true}}>
+        <div />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div attr2="ATTR 2" />
+    ));
+  });
+
+  test('filter out forbidden attributes', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .attr = ATTR
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo" attrs={{attr: false}}>
+        <div />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div />
+    ));
+  });
+
+  test('filter all attributes if attrs not given', function() {
     const mcx = new MessageContext();
     const l10n = new ReactLocalization([mcx]);
 
@@ -59,6 +102,27 @@ foo
 
     const wrapper = shallow(
       <Localized id="foo">
+        <div />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div />
+    ));
+  });
+
+  test('preserve existing attributes when setting new ones', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .attr = ATTR
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo" attrs={{attr: true}}>
         <div existing={true} />
       </Localized>,
       { context: { l10n } }
@@ -69,6 +133,69 @@ foo
     ));
   });
 
+  test('overwrite existing attributes if allowed', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .existing = ATTR
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo" attrs={{existing: true}}>
+        <div existing={true} />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div existing="ATTR" />
+    ));
+  });
+
+  test('protect existing attributes if setting is forbidden', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .existing = ATTR
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo" attrs={{existing: false}}>
+        <div existing={true} />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div existing={true} />
+    ));
+  });
+
+  test('protect existing attributes by default', function() {
+    const mcx = new MessageContext();
+    const l10n = new ReactLocalization([mcx]);
+
+    mcx.addMessages(`
+foo
+    .existing = ATTR
+`)
+
+    const wrapper = shallow(
+      <Localized id="foo">
+        <div existing={true} />
+      </Localized>,
+      { context: { l10n } }
+    );
+
+    assert.ok(wrapper.contains(
+      <div existing={true} />
+    ));
+  });
+
   test('$arg is passed to format the value', function() {
     const mcx = new MessageContext();
     const format = sinon.spy(mcx, 'format');
