fix(inputs): do not generate new field id on each render (#999)

* fix(inputs): do not generate new field id on each render

Author: montezume

src/components/fields/password-field/password-field.js

@@ -3,13 +3,13 @@ import PropTypes from 'prop-types';
 import { useIntl } from 'react-intl';
 import requiredIf from 'react-required-if';
 import useToggleState from '../../../hooks/use-toggle-state';
+import useFieldId from '../../../hooks/use-field-id';
 import Constraints from '../../constraints';
 import Spacings from '../../spacings';
 import FieldLabel from '../../field-label';
 import PasswordInput from '../../inputs/password-input';
 import FlatButton from '../../buttons/flat-button';
 import { EyeIcon, EyeCrossedIcon } from '../../icons';
-import getFieldId from '../../../utils/get-field-id';
 import createSequentialId from '../../../utils/create-sequential-id';
 import FieldErrors from '../../field-errors';
 import filterDataAttributes from '../../../utils/filter-data-attributes';
@@ -22,7 +22,7 @@ const hasErrors = errors => errors && Object.values(errors).some(Boolean);
 const PasswordField = props => {
   const intl = useIntl();
   const [isPasswordVisible, togglePasswordVisibility] = useToggleState(false);
-  const id = getFieldId(props, {}, sequentialId);
+  const id = useFieldId(props.id, sequentialId);
   const hasError = props.touched && hasErrors(props.errors);
 
   return (

src/components/inputs/localized-money-input/localized-money-input.js

@@ -13,7 +13,7 @@ import {
   getId,
   getName,
 } from '../../../utils/localized';
-import getFieldId from '../../../utils/get-field-id';
+import useFieldId from '../../../hooks/use-field-id';
 import createSequentialId from '../../../utils/create-sequential-id';
 import filterDataAttributes from '../../../utils/filter-data-attributes';
 import MoneyInput from '../money-input';
@@ -131,7 +131,7 @@ const LocalizedMoneyInput = props => {
     defaultExpansionState
   );
 
-  const id = getFieldId(props, {}, sequentialId);
+  const id = useFieldId(props.id, sequentialId);
 
   const hasErrorInRemainingCurrencies =
     props.hasError ||

src/components/inputs/localized-money-input/localized-money-input.story.js

@@ -45,7 +45,7 @@ storiesOf('Components|Inputs', module)
               key={key}
               id={text('id', undefined)}
               name={text('name', 'productName')}
-              value={object('value', value)}
+              value={value}
               onChange={event => {
                 action('onChange')(event);
                 onChange({

src/components/inputs/localized-text-input/localized-text-input.js

@@ -5,6 +5,7 @@ import { oneLine } from 'common-tags';
 import { FormattedMessage } from 'react-intl';
 import { css } from '@emotion/core';
 import useToggleState from '../../../hooks/use-toggle-state';
+import useFieldId from '../../../hooks/use-field-id';
 import ErrorMessage from '../../messages/error-message';
 import filterDataAttributes from '../../../utils/filter-data-attributes';
 import Spacings from '../../spacings';
@@ -21,7 +22,6 @@ import {
   getId,
   getName,
 } from '../../../utils/localized';
-import getFieldId from '../../../utils/get-field-id';
 import createSequentialId from '../../../utils/create-sequential-id';
 import LanguagesButton from './languages-button';
 import messages from './messages';
@@ -136,7 +136,7 @@ const LocalizedTextInput = props => {
     Object.keys(props.value)
   );
 
-  const id = getFieldId(props, {}, sequentialId);
+  const id = useFieldId(props.id, sequentialId);
 
   const hasErrorInRemainingLanguages =
     props.hasError ||

src/components/inputs/time-input/time-input.js

@@ -1,8 +1,9 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 import { useIntl } from 'react-intl';
+import useFieldId from '../../../hooks/use-field-id';
+import usePrevious from '../../../hooks/use-previous';
 import filterDataAttributes from '../../../utils/filter-data-attributes';
-import getFieldId from '../../../utils/get-field-id';
 import createSequentialId from '../../../utils/create-sequential-id';
 import Constraints from '../../constraints';
 import { parseTime } from '../../../utils/parse-time';
@@ -24,8 +25,10 @@ const format24hr = ({ hours, minutes, seconds, milliseconds }) => {
 const hasMilliseconds = parsedTime => parsedTime.milliseconds !== 0;
 
 const TimeInput = props => {
-  const id = getFieldId(props, {}, sequentialId);
+  const id = useFieldId(props.id, sequentialId);
   const intl = useIntl();
+  const prevLocale = usePrevious(intl.locale);
+
   const { name, value, onBlur, onChange } = props;
 
   const emitChange = React.useCallback(
@@ -53,15 +56,10 @@ const TimeInput = props => {
 
   const onClear = React.useCallback(() => emitChange(''), [emitChange]);
 
-  // so that it doesn't run on initial mount
-  const mounted = React.useRef();
-  React.useEffect(() => {
-    if (!mounted.current) {
-      mounted.current = true;
-    } else {
-      emitChange(TimeInput.toLocaleTime(value, intl.locale));
-    }
-  }, [intl.locale, emitChange, value]);
+  // if locale has changed
+  if (typeof prevLocale !== 'undefined' && prevLocale !== intl.locale) {
+    emitChange(TimeInput.toLocaleTime(value, intl.locale));
+  }
 
   return (
     <Constraints.Horizontal constraint={props.horizontalConstraint}>

src/components/tooltip/tooltip.js

@@ -6,7 +6,7 @@ import styled from '@emotion/styled';
 import isNil from 'lodash/isNil';
 import usePopper from 'use-popper';
 import useToggleState from '../../hooks/use-toggle-state';
-import getFieldId from '../../utils/get-field-id';
+import useFieldId from '../../hooks/use-field-id';
 import createSequentialId from '../../utils/create-sequential-id';
 import { Body, getBodyStyles } from './tooltip.styles';
 
@@ -47,7 +47,7 @@ const Tooltip = props => {
 
   const isControlled = !isNil(props.isOpen);
   const tooltipIsOpen = isControlled ? props.isOpen : isOpen;
-  const id = getFieldId(props, {}, sequentialId);
+  const id = useFieldId(props.id, sequentialId);
 
   const { onClose } = props;
   const handleClose = React.useCallback(
@@ -188,6 +188,7 @@ Tooltip.propTypes = {
   }).isRequired,
   off: PropTypes.bool.isRequired,
   isOpen: PropTypes.bool,
+  id: PropTypes.string,
   onClose: PropTypes.func,
   onOpen: PropTypes.func,
   placement: PropTypes.oneOf([

src/hooks/use-field-id/index.js

@@ -0,0 +1 @@
+export { default } from './use-field-id';

src/hooks/use-field-id/use-field-id.js

@@ -0,0 +1,14 @@
+import React from 'react';
+import getFieldId from '../../utils/get-field-id';
+
+const useFieldId = (id, sequentialId) => {
+  const [internalId, setId] = React.useState(id);
+
+  React.useEffect(() => {
+    setId(getFieldId({ id }, { id: internalId }, sequentialId));
+  }, [id, internalId, setId, sequentialId]);
+
+  return internalId;
+};
+
+export default useFieldId;

src/hooks/use-field-id/use-field-id.spec.js

@@ -0,0 +1,43 @@
+import React from 'react';
+import { render } from '../../test-utils';
+import createSequentialId from '../../utils/create-sequential-id';
+import useToggleState from '../use-toggle-state';
+import useFieldId from './use-field-id';
+
+const sequentialId = createSequentialId('test-id-');
+
+const TestComponent = props => {
+  // eslint-disable-next-line react/prop-types
+  const [isToggled, toggle] = useToggleState(false);
+  // eslint-disable-next-line react/prop-types
+  const id = useFieldId(props.id, sequentialId);
+
+  return (
+    <div id={id}>
+      <button data-testid="toggle-btn" onClick={toggle}>
+        Toggle
+      </button>
+      <div data-testid="test-ctn">{isToggled ? 'Foo' : 'Bar'}</div>
+    </div>
+  );
+};
+
+describe('when id not provided', () => {
+  it('should use sequential-id and not increment on rerender', () => {
+    const { container, getByTestId } = render(<TestComponent />);
+    expect(container.querySelector('#test-id-1')).toBeInTheDocument();
+    getByTestId('toggle-btn').click();
+    expect(container.querySelector('#test-id-1')).toBeInTheDocument();
+    getByTestId('toggle-btn').click();
+    expect(container.querySelector('#test-id-1')).toBeInTheDocument();
+  });
+});
+
+describe('when id is provided', () => {
+  it('should use provided id and not change on rerender', () => {
+    const { container, getByTestId } = render(<TestComponent id="foo-bar" />);
+    expect(container.querySelector('#foo-bar')).toBeInTheDocument();
+    getByTestId('toggle-btn').click();
+    expect(container.querySelector('#foo-bar')).toBeInTheDocument();
+  });
+});