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

Make defaultMessage warning message configurable #251

Closed
ratson opened this issue Dec 17, 2015 · 17 comments
Closed

Make defaultMessage warning message configurable #251

ratson opened this issue Dec 17, 2015 · 17 comments

Comments

@ratson
Copy link

ratson commented Dec 17, 2015

Continue the discussion in 8a78992#commitcomment-14989589

defaultMessage is often use as the primary source for translation, since it is already bundle with the application, and likely no need for translation, it should be a valid use-case that not loading messages to avoid duplicates.

@ericf
Copy link
Collaborator

ericf commented Dec 17, 2015

#162 (comment)

So we used defineMessages, but we didn't extract them. Now we have to do that.

@janhancic are you only extracting messages now because of the warnings in the console in development? Or is this just forcing you to add this step to your build now instead of in the future?

@ericf ericf mentioned this issue Dec 17, 2015
7 tasks
@janhancic
Copy link

Pretty much yeah. We might add another language at some point in the future, but we don't actually have any plans to do so. So our main use right now is just to separate text from our components and future proof the app. So having to do this right now is a bit annoying to be honest :)

@ericf
Copy link
Collaborator

ericf commented Dec 18, 2015

@janhancic for single language apps I recommend not using messages since it's inherently more complex, and instead use the other <Formatted*> components, including <FormattedPlural>.

The future-proofing argument is interesting, but I don't have a good solution for that you and @ratson are asking for right now that's not also a footgun.

Feel free to use the code in examples/ for collating the defaultMessage extracted from babel-plugin-react-intl:

import * as fs from 'fs';
import * as p from 'path';
import {sync as globSync} from 'glob';
import {sync as mkdirpSync} from 'mkdirp';

const MESSAGES_PATTERN = './build/messages/**/*.json';
const LANG_DIR         = './build/lang/';

// Aggregates the default messages that were extracted from app's React 
// components via the React Intl Babel plugin. An error will be thrown if there 
// are messages in different components that use the same `id`. The result is a 
// flat collection of `id: message` pairs for the app's default locale.
let defaultMessages = globSync(MESSAGES_PATTERN)
    .map((filename) => fs.readFileSync(filename, 'utf8'))
    .map((file) => JSON.parse(file))
    .reduce((collection, descriptors) => {
        descriptors.forEach(({id, defaultMessage}) => {
            if (collection.hasOwnProperty(id)) {
                throw new Error(`Duplicate message id: ${id}`);
            }

            collection[id] = defaultMessage;
        });

        return collection;
    }, {});

mkdirpSync(LANG_DIR);
fs.writeFileSync(
    p.join(LANG_DIR, 'en-US.json'), 
    JSON.stringify(defaultMessages, null, 2)
);

@ratson
Copy link
Author

ratson commented Dec 20, 2015

I end up monkey patching console.error to suppress the warnings. For those who are not ready to add messages prop yet, here is a hacky snippet to rescue.

if (process.env.NODE_ENV !== 'production') {
  const originalConsoleError = console.error
  if (console.error === originalConsoleError) {
    console.error = (...args) => {
      if (args[0].indexOf('[React Intl] Missing message:') === 0) {
        return
      }
      originalConsoleError.call(console, ...args)
    }
  }
}

@janhancic
Copy link

@ericf that's the code is took ys, ta. We're going to continue using this, as we have an app with insane amount of copy in it, so we wanted to extract it out from our markup and as a added benefit get the future proofing, as we know that at some point it's going to have to be internationalised.

As an additional piece of information, we actually split out our messages into a separate file, which we then import into the component(s) that use those messages. So you end up with code like this:

MyComponent.js

import React from 'react';
import messages from './MyComponent.messages';

class MyComponent extends Component {
  ...
}

MyComponent.messages.js

import {defineMessages} from 'react-intl';

export default defineMessages({
 ...
});

Maybe it's just because we have so much copy, but we find it better to not have the actual text in the component file as it makes it much more readable and easy to maintain. It's not a hard rule though, so some smaller components might have the text inline (although you lose the ability to re-use the messages that way).

@jakubkarlicek
Copy link

I see the point. I'd also like to use defaultMessage as primary translation, rather than downloading same language data again.

@ericf
Copy link
Collaborator

ericf commented Dec 21, 2015

@janhancic with big swaths of text, <FormattedMessage> might not be the best solution — it excels better at UI strings, and not as much for content strings. For content, e.g. "About Us" section, pulling it out of the React components makes sense to me since it's not part of the UI.

@khouba understood.

I'm wondering if the improvement that I can do here is no warn when locale === defaultLocale. I think it could handle this case well. The main reason for the warnings are when you're testing out a locale with translated messages and one is missing.

@janhancic
Copy link

We have a bit of both I guess (short UI text and copy), so it just makes sense to use the same thing to handle both.

ericf added a commit to ericf/react-intl that referenced this issue Dec 21, 2015
This prevents warnings from littering the console in development
when no `messages` are passed into the <IntlProvider> for the default
locale, and a default message is in the source.

Fixes formatjs#251
@ericf
Copy link
Collaborator

ericf commented Dec 21, 2015

Here's a PR that should resolve this issue without having to add a config option: #253

@ericf
Copy link
Collaborator

ericf commented Dec 21, 2015

@janhancic okay, sounds good. Keep us updated on how well it works for the copy use-case. For UI strings you could define them in the components and leave the copy strings defined in their own module like you showed in your example code above.

@janhancic
Copy link

Sweet, thank you very much. Are you planning on doing a new release soon with the above PR?

@janhancic
Copy link

Hey @ericf happy new year :)

Just curious if you know when this change will be released?

@ericf
Copy link
Collaborator

ericf commented Jan 4, 2016

No date planned yet.

@Kosta-Github
Copy link

What you could also do, is add an additional locale entry either as a second argument of (my favorite):

defineMessages(
    ...,
    locale = 'en'
);

or to each individual message definition:

{
    id:             ...,
    description:    ...,
    defaultMessage: ...,
    locale:         'en'
}

If this locale is present, you could assume that the defaultMessage is already the appropriate translation/message for that locale.

This would actually colocate the messages with the locale info they are defined for/in. Using the IntlProvider.defaultLocale is tearing that information apart and also does a bit of black magic when comparing it with this.locale (in whatever context)...

@catamphetamine
Copy link

I am also having these errors in the console.
I would like to avoid generating that translations file for english language.

@catamphetamine
Copy link

So I installed that babel-plugin-react-intl plugin and set it up in .babelrc and it outputs .json files into build/messages folder.
What to do next?
How is this all imported into the application?

@catamphetamine
Copy link

I suppose the resulting .json files have to be further converted into a proper translations file like that
https://github.com/yahoo/react-intl/blob/master/examples/translations/scripts/translate.js

That's too complicated.
And I care for my SSD drive so I don't want it constantly rewriting itself every time I hit Ctrl + S.

So, I'm removing that Babel plugin from the chain and keep it simple.
@ratson thanks for the solution, it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants