Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor nested example to move <IntlProvider> into widget #235

Closed
wants to merge 1 commit into from
Closed

Refactor nested example to move <IntlProvider> into widget #235

wants to merge 1 commit into from

Conversation

baer
Copy link
Contributor

@baer baer commented Dec 3, 2015

Refactor the example to move the <IntlProvider> inside of the widget. Without this, the widget will be unusable as a standalone component.

Note: This is best viewed with the whitespace flag. Indenting made the diff look much bigger than it is.

@baer baer changed the title Refactor nested example to move <IntoProvider> into widget Refactor nested example to move <IntlProvider> into widget Dec 3, 2015
@ericf
Copy link
Collaborator

ericf commented Dec 7, 2015

If we do this, the Widget should also take a locale prop, otherwise it's not as useful if used standalone.

@baer
Copy link
Contributor Author

baer commented Dec 8, 2015

You're right. Fixed it!

@ericf
Copy link
Collaborator

ericf commented Dec 9, 2015

@baer curious to hear more about what's leading you towards putting the nested <IntlProvider> inside the a Widget vs. having the Widget require that one be provided somewhere in its ancestry. I think you've thought about this more than me and it would be good if there's "one true way" I can document about how to do this.

@baer
Copy link
Contributor Author

baer commented Dec 9, 2015

Well, it all boils down to this: I write widgets every day that need to be consumed by an wide variety of consumers.

They generally fall into 3 categories and while I tried to be specific about our needs in the descriptions I think these categories apply generally to lots of companies/projects. It's just easier to give concretes examples than hypotheticals :)

  1. Consumers that already use React-Intl - These tend to be customer facing and under my team's umbrella. I have no issue asking/helping them to make modifications to their app (IntlProvider in the ancestry) and often I can do it for them.
  2. Consumers that use a completely different l10n/i18n toolchain - These projects are not under my team's umbrella and have often come to React on their own. This could be an acquisition, a BI app that is rolling out to a new LOB or any number of other scenarios. Often these teams have lots of their own tooling and infra. Since my group generally has way more resources on similar problems I need to enable them to use some of the widgets that we build to prevent duplication of effort. More specifically I need a way to provide them with widgets that they can localize using their own tools. In practice this just means adding messages/locale as props that they can pass data to however they like. It is generally an impediment to adoption to ask them to take on new dependencies or change their app code too much.
  3. Consumers that have 0 interest in localizing their applications - BI for example is a whole set of apps that (ordinarily) doesn't have a lot of use for localization and is perfectly happy with the default messages and simple date/time formatting. For these people it does not make sense to ask them to take on two dependencies and wrap every widget since React-Intl is an irrelevant detail to them.

It also IMHO indicates a a leaky abstraction to have to ask a widget's consumer to take a dependency on an i18n lib since it's an implementation detail.

For Consumers 2 and 3, I've chosen to build widgets with their own IntlProvider.

@ericf
Copy link
Collaborator

ericf commented Dec 9, 2015

For consumers 2 it still leaks out that React Intl is being used because they'll have to load React Intl's locale data for the locale they want to render in. Makes sense for 3 though, assuming English-only.

@baer
Copy link
Contributor Author

baer commented Dec 9, 2015

Re: Consumer 2 I didn't think of that... The answer for me might be that all widgets include all locales and I do some pruning in my build step. Without having the consumer register locale data I'm not sure how to handle that. Moment.js has given this a lot of thought and their solution is erm... uhhh... not great :).

@ericf
Copy link
Collaborator

ericf commented Dec 9, 2015

Also, you likely don't want end up with multiple copies of a library like React Intl, so some widget developers might include it in peerDependencies, which forces the consumer to deal with add it to their dependencies.

@baer
Copy link
Contributor Author

baer commented Dec 10, 2015

In npm@3 you shouldn't have multiple copies eve if it's not a peerDependency. Maybe you've thought of something I haven't though. Can you elaborate on that?

@ericf
Copy link
Collaborator

ericf commented Dec 10, 2015

In our internal "Widget" setup we're putting React Intl in peerDependencies and then having the root app depend on it. We're also sticking <IntlProvider> instances around the Widgets, but this is totally consumer 1 stuff from your list of three. Our framework takes care of creating the <IntlProvider> and the nested ones around each Widget. For testing a Widget, we have a wrapper which includes <IntlProvider>.

Consumer 2 seems to be the most interesting… that's why I was curious to hear more about how you're handling that case. I'm on board with pulling this in this PR, maybe I'll make the example show both forms (<IntlProvider> outside vs inside)… or do you think that will confuse people even more? 😄

@baer
Copy link
Contributor Author

baer commented Dec 10, 2015

Some details on the Consumer 2 case. Actually, since writing Consumer 2 is going to get annoying I'm going to call them External Integrators. I'll speak in detail about our (Walmart) use case but I believe that many of these things will be true in a more general setting.

Walmart like Facebook and Microsoft, has a wide range of business units with teams that work mostly independently and are each responsible for their own technical decisions / stack. This allows for a somewhat darwinian approach to development and tooling where the truly good things tend to spill over into other teams. Usually when one team latches onto something excellent they try to spread the good word. This is actually how React came to be at Facebook (but I'm sure you already knew that).

The reason I mention all of this is that every interaction with other teams (External Consumers) expends some amount of political capital which means that in order to be successful the new tools have to be demonstrably better than what they had and have a low barrier to entry. The more I ask these consumers to change their build process, add dependencies etc. the harder the sell is and the slower the rollout.

This is a stripped down version of how one of my Widgets looks:

components/greeting.jsx

"use strict";

import React from "react";
import {IntlProvider, FormattedMessage, defineMessages} from 'react-intl';

// All messages MUST be defined in a defineMessages block. Any messages defined 
// elsewhere will throw errors from a custom ESLint plugin. Having 1 way to do things
// is very important since contributors come from many teams both in the US and 
// abroad. Communicating simple rules is paramount to making tooling effective.
const defaultMessages = defineMessages({
  "greeting": {
    id: "greeting",
    description: "A friendly greeting for a user visiting the homepage",
    defaultMessage: "Hello There!"
  }
});

export default React.createClass({
  propTypes: {
    messages: PropTypes.object,
    locale: PropTypes.string
  },

  render() {
    // All components that are exposed through (require("package.json")).main MUST be
    // wrapped in an IntlProvider. This ensures the component is useful for Consumers 
    // 2 and 3 discussed above
    return (
      <IntlProvider messages={this.props.messages}>
        <FormattedMessage {...defaultMessages["greeting"]}/>
      </IntlProvider>
    );
  }
});

index.js

module.exports = {
  Greeting: require("./dist/components/greeting"),
  // This is the flattened result of running the `babel-plugin-react-intl` All keys are 
  // prefixed with a SHA hash (a babel plugin I just wrote) to prevent collisions in
  // an External Consumer's messages file since I have to assume they will want
  // to roll messages up into a single bundle in their build process.
  messages: require("dist/messages-raw.json")
};

I hope this gives you a better idea of what I'm trying to accomplish. Let me know if you have any questions.

@ericf
Copy link
Collaborator

ericf commented Dec 11, 2015

@baer yeah it is! How are you documented that the React Intl locale data must be loaded on the page, along with the Intl.js polyfill for consumers 2 & 3?

@baer
Copy link
Contributor Author

baer commented Dec 11, 2015

Honestly, I don't have good answers to those questions yet. It seems like all bad options so I was kindof punting on the question for now 😕.

Polyfill

  • Polyfill in the widget - This is horrible practice and if I were reviewing vendor code that was polyfilling I'd probably not integrate
  • Put a note in the docs - People don't read docs until things break. Copy Paste for life! Anyway, I just foresee this not going terribly well.
  • PeerDependency - It just feels sloppy to leak implementation detail but I probably will end up here...

locale-data

  • I'm pretty sure that I'm going to pull a moment.js and just load them all in my widget. It's easy enough to use a webpack plugin to remove cruft if I find that uglify doesn't do the job.

If you have any ideas I'm all ears :).

Also, thank you again for all the amazing work you do. Your responses are always really thoughtful and on point and the work you've been putting out is exceptional. I just wrote my first babel-plugin and it's a freaking abyss filled with missing documentation. You're killin' it with babel-plugin-react-intl too!

@ericf
Copy link
Collaborator

ericf commented Dec 11, 2015

Thanks for these kinds words! 😄

I'm literally working on the exact same problem within Yahoo. But we're much more in the consumer 1 use case, so it's easier for our framework to provide the nested <IntlProvider> wrapper around each Widget and feed it only its own messages. This is why I had the example setup where the <App> component was creating the nested <IntlProivder>.

The consumer 3 story is slightly easier (assuming English-only), because they'd just need to load the Intl.js polyfill in older browsers. I'm working on some tweaks to this example with your changes now… I'll get it in this weekend if I don't finish now.

@ericf
Copy link
Collaborator

ericf commented Dec 11, 2015

Sent a PR against your branch: https://github.com/baer/react-intl/pull/1

@dnutels
Copy link

dnutels commented Mar 16, 2016

I'd like to do two things, if you don't mind:

  1. congratulate on an absolutely excellent library and general approach to things
  2. chime in with a question and/or clarification request on the Widget/App front

Perhaps this would somehow help with the docs... so as to not waste your time completely.

This is the setup:

components/some-widget.js

export default class SomeWidget extends Component {
    render() {
        // This is the line that bothers me...
        const {messages} = this.props.intl || this.props;

        return (
            <IntlProvider messages={messages}>
                <div>
                    <h1><FormattedMessage {...defaultMessages.heading} /></h1>
                </div>
            </IntlProvider>
        );
    }
}

And, somewhere up in the hierarchy:

components/some-app-route.js

const InjectedSomeWidget = injectIntl(SomeWidget);

export default class SomeAppRoute extends Component {
    render() {
        return (
            <div>
                <InjectedSomeWidget/>
            </div>            
        );
    }
}

and finally, at the very top (presumably):

index.js

<IntlProvider locale="en" messages={messages}>
    <Router history={history}>
        <Route path="/" component={App}>
            ...
            <Route path="data" component={SomeAppRoute} />
            ...
        </Route>
    </Router>
</IntlProvider>

It seems to me that this is a proper usage aside from the line above in SomeWidget:

 const {messages} = this.props.intl || this.props;

Would you say this is correct? And that this is the intended usage of the nested IntlProviders?

Thank you.

@redonkulus
Copy link
Member

@baer Just reviewing old PRs, do you still want to pursue this or should we close it?

@baer
Copy link
Contributor Author

baer commented Sep 17, 2018

Probably time to close it out.

@baer baer closed this Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants