Update doc in developer guide on when to use injectIntl vs contextTypes vs FormattedMessage component and standardize testing these different approaches about terra-ui HOT 18 CLOSED

If one approach is generally preferred over others going forward, it could be particularly important to reflect that in the component starter as many teams use that as a source of truth for the community standard.

from terra-ui.

You can use the render prop version of FormattedMessage for the case where you need a string. eg

<FormattedMessage id="Connect.Alerts.someIntlId" >
  {(alertText) =>
    <Alert
      type={Alert.Opts.Types.INFO}
      title={alertText} // Not a String type
    />
  }
</FormattedMessage>

Not the most concise option, but still saves you from using injectIntl.

from terra-ui.

React does recommend using their new contextType specification for simple components that use a single Context consumer. I think we should recommend using it when possible. We would just need to ensure that the actual Context objects are exported from the packages that have context APIs. I'm not sure if react-intl will be exporting their Context object in the future. If they don't, we should request it.

One thing to keep in mind going forward is that components will likely be consuming more than one context in the future, which will necessitate wrappers until alternatives like the useContext hook are available. Personally, I prefer wrapping in these situations to using nested Consumers in a component's render block, especially for class components that may need those values outside of render. We'll be providing withBlah() component generators for that purpose within Terra.

Testing issues can be mitigated by exporting the base unwrapped component for testing (if diving through WrappedComponents n-times doesn't float your boat). These wrappers aren't actually producing DOM markup; they certainly bloat the component tree in the React dev tools, but some of the confusion can be helped by including decent displayNames for those wrapper components. I'm finding that component tree-view less and less useful for debugging our larger applications regardless.

from terra-ui.

In my mind, I'm leaning towards recommending <FormattedMessage /> first, if you need a string, use the render prop version, if get into a situation where you are nesting multiple render prop based <FormattedMessage /> components, use injectIntl.

I'm curious to hear what @tbiethman, @JakeLaCombe, @nickpith, @dev-cprice, @noahbenham, and @RHolland13 think about this approach.

The other part of this issue would be documenting how to test these different approaches with Jest/Enzyme.

I know @nickpith recently worked on a contribution to https://github.com/joetidee/enzyme-react-intl that makes loading translations optional.

Rather than using a real translation file or a mock object where I have to define all the possible keys. It would be easier to use if the default behavior of the *WithIntl methods just used the translation key as the value rendered in a component during testing.

I'd like to dig into this approach some more as I think it will reduce the testing complexity, shoudn't need to call aggregate-translations in jest setup anymore with this I believe.

from terra-ui.

The crux of the issue is more about testing the component in isolation vs. writing a full integration test.

In a shallow-mounted snapshot, components that render a FormattedMessage aren't going to actually include the translated string in their snapshots (I think? Correct me if I'm wrong here). Nor should they, because interfacing with the intl context is a concern of the FormattedMessage component. You just care about giving it the right string id prop.

Components that do use injectIntl will mostly likely render out the translated string, but they don't necessary care what that string is. They just care that they're getting it from a function called formatMessage on a prop called intl, which could be mocked out for testing purposes.

Writing jest tests this way would require wdio tests covering all workflows that utilize translations, but generally that'd be expected anyway (unless you're fully mounting your components for snapshots, which will have its own problems for higher-order components).

from terra-ui.

We've preferred the contextTypes strategy so far because it eliminates the extra wrapper around our components. This is especially relevant with Reduxified components that already have a wrapper.

So with the current legacy context,

export default class MyComponent extends React.Component {
  static contextTypes = { intl: () => {} };
  
  render() {
    <div>{this.context.intl.formatMessage({ id: '...' })</div>
  }
}

I believe the migration path forward to the new context API using this strategy could look something like

import ContextType from 'terra-base/.../ContextType';

export default class MyComponent extends React.Component {
  static contextType = ContextType;
  
  render() {
    <div>{this.context.intl.formatMessage({ id: '...' })</div>
  }
}

The FormattedMessage strategy wasn't universally effective for us because it returns a react component. There are many places where a plain text string is preferred. For example, React would throw a prop type warning doing this:

<Alert
  type={Alert.Opts.Types.INFO}
  title={<FormattedMessage id="Connect.Alerts.someIntlId" />} // Not a String type
  />

from terra-ui.

I think that it's more than just which of the three approaches we should be using.

I propose that we stop restricting Terra component props as strings if they're for presentational purposes. For example, we could pass <FormattedMessage /> directly to something like the text prop of terra-button if it was PropTypes.node.isRequired instead of PropTypes.string.isRequired:

<Button text={<FormattedMessage id="SOME_ID_HERE" />} />

The downside of this? People can now pass whatever they want besides just strings to old string props. I say: so what? Devs are already mangling components, so let's just document proper usage and make it much easier for us to consume Terra components!

This would alleviate so much developer friction!! ❤️❤️❤️

Having to specify contextTypes means that you would need to use React.Components (excluding the awesome upcoming hooks API, useContext) or Context render props for functional components:

<YourContext.Consumer>
  {yourContext => (
    ...
  )}
</YourContext.Consumer>

I think having injectIntl usage all over the place sucks for multiple reasons (it makes your DOM tree ugly and testing annoying), so it's the worst option, by far

EDIT:
I bring this up mostly because I'd say about 90% of the components I've made from terra components are functional instead of class based. At least, the first time I write a component, I write it without state (again, this is before the intro of hooks). Functional components are much more terse and easier to reason about.

from terra-ui.

This is really good feedback. I think FormattedMessage can be good solution, but I'm unsure about always recommending it. There are some props we need to keep as strings due to the fact that the underlying HTML attribute we pass the prop to only accepts string. As a contrived example, let's say we have a component like the following where label and placeholder map directly the HTML attributes on an input.

<Input label={localeLabel} placeholder={localePlaceholder) />

Using the render props version of <FormattedMessage />, we could get around this with nested <FormattedMessage />, but you can see how only using <FormattedMessage /> could lead to deeper nesting as well.

<FormattedMessage id="input-label">
  { (label) =>
        <FormattedMessage id="input-placeholder>
            { (placeholder) =>
                <Input label={label} placeholder={placeholder} />
            }
        </FormattedMessage>
  }
</FormattedMessage>

In this case, using injectIntl offers a terser bit of code:

import { injectIntl } from 'react-intl'

const I18nLabel = ({ labelId, placeholderIntl, intl }) => (
  <Input
    label={intl.formatMessage({ id: labelId })}
    placeholder={intl.formatMessage({ id: placeholderId })}
   />
)

export default injectIntl(I18nLabel)

The original maintainer of react-intl has noted that:

Using context directly works today, but not recommend because it could change in the future. The officially supported way is to wrap your component with the injectIntl() HoC.

So I think we should be moving away from contextTypes.

from terra-ui.

That's great perspective, thank you @tbiethman. Out of curiosity, what other context types would you expect in the future?

from terra-ui.

In theory, I like the idea of making loading translations optional as it reduces complexity. However, I'm also concerned it would reduce test quality. I've made it a point to explicitly test string translations in our component tests to ensure the requested key exists in the real translation file and is still the expected value. Correlating these leads to stronger tests in my opinion.

from terra-ui.

@bjankord That recommendation makes sense to me.

@noahbenham While Context usage has fewer use cases for atomic components (intl being the main one right now), I see Context being heavily used in application architectures for broad communication needs. For example, there's a PR out now for the terra-disclosure-manager that converts it to a context-based API: cerner/terra-framework#318.

Previously, the DisclosureManager utilized prop injection to provide its APIs. The issue with prop injection at scale is that it is obtrusive to components that neither need the functionality nor expect that prop to be jammed into their component. Consumers end up asking themselves "Where is this prop coming from?" and "Why do I have to pass this value to my child components that need it? How do I know what components need it?". These questions don't have helpful answers, and the problems only get worse as component trees get deeper and deeper.

Moving to Context requires components that consume those APIs to opt-in to using them and allows components that don't to ignore it altogether. This is much less obtrusive to consumers while providing the same functionality, and I think it allows Terra to provide some semblance of framework-level components without being too restrictive to consumers.

Regarding your comment above about using translated string values in tests, could you validate those same strings using wdio visual regression vs. within your jest snapshots? It'd be ideal to test in both places, but given the increased complexity, limiting your testing concerns within Jest (if possible) seems appropriate.

from terra-ui.

I haven't seen the implementation of context in DisclosureManager (out of the office until Tues, on mobile) but I very much like that approach. The current API contract feels cumbersome.

We could test this with wdio but I question whether the overhead introduced by testing every iteration of a component's possible string displays with wdio is an improvement over the current complexity of calling aggregateTranslations in jestSetup.

Are there other hidden complexities we're trying to solve for by eliminating translation loading in Jest tests?

from terra-ui.

The main issue with translations being included during jest/enzyme tests is when you have snapshots with multiple components including the intl object in them. Adding a new translation KVP or updating an old one means the intl.messages object is different which means that all snapshots are updated, regardless if you've touched them. It seems odd to have <Foo />s snapshot depend on an update to <Bar />s i18n messages. We've run into issues in the past where creating a new component (which has additional KVPs in the translations file) updates 10 or more snapshots which bloats a review. Eventually, people start ignoring snapshot changes, which is a HUGE problem. They are highly valuable when used properly and I want to make sure they maintain their value/worth.

I definitely see the value in making sure that a key actually exists for a component, but I'm not sure how to easily solve that which the above problem in mind. 🤔

I now want to write a test package that traverses all components and ensures that all translations provided have the desired KVPs.. 😲best of both worlds? #hackathon

EDIT:
I guess the last point is kind of an enhancement to the aggregateTranslations script, in a way..

from terra-ui.

My concern with mocking around formatMessage is it doesn't correlate the ID in your component to the value in the actual JSON. You could easily change the key/values in ‘en.json‘ and your tests would happily pass.

I can definitely see how snapshot testing is hindered by real translations. However, is that only a problem when using injectIntl? We've been happily writing Jest snapshot tests for our components that use ‘contextTypes‘ without issue.

It seems optionally loading translations in tests could be preferable when testing components using ‘injectIntl‘ but I wonder if this should be universally applicable for consumers using ‘contextTypes‘, if that's a strategy we want to support.

from terra-ui.

+1 to @bjankord recommended approach to using <FormatMessage /> when it is possible to render a component and then using injectIntl() when a string prop is required. This aligns with react-intl’s definition of injectIntl’s usage

React Intl provides an API to inject the imperative formatting API into a React component via its props. This should be used when your React component needs to format data to a string value where a React element is not suitable; e.g., a title or aria attribute, or for side-effect in componentDidMount.

from terra-ui.

+1 to all @bjankord's suggestions.

In my opinion, the usage of old contextTypes or new contextType to get the intl object should be stopped because it is exposing the actual implementation details of react-intl. All usage should be based on react-intl guidelines and if you need access to the intl object, you should use injectIntl() as they intend. I don't believe them make any guarantees from an API perspective that intl is in the context.

from terra-ui.

Noting some different ways to test with react-intl:

• https://github.com/joetidee/enzyme-react-intl
• https://blog.theodo.fr/2017/12/clean-tests-react-intl-missing-message-errors-console/

from terra-ui.

Resolved in #132

from terra-ui.