dojo / i18n

:rocket: Dojo 2 - internationalization library.
http://dojo.io
Other
6 stars 19 forks source link

Implement locale-specific bundle loading and `Stateful` registry. #11

Closed mwistrand closed 8 years ago

mwistrand commented 8 years ago

This PR includes the following i18n features:

Other features that will be added in the future pending discussions are message/date/time formatting and incorporation into the dojo-app ecosystem (I will create additional issues as needed).

@agubler had mentioned that it would be nice to have intellisense on i18n bundles. Given that these will be TypeScript modules, and since the bundle's default module will always be loaded for locale data, this could be accomplished by loading bundle defaults directly as normal modules instead of going through the loader (e.g., import messages, { locales } from 'nls/main';). However, we would then need a way to register that module with the i18n system so that it does not attempt to reload that package when the locale is switched or when Statefuls are registered. I am open to suggestions here.

jquerybot commented 8 years ago

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

:memo: Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

kitsonk commented 8 years ago

Just to summerise a discussion that we had between @mwistrand @agubler and myself... looking at the API from a consumer's perspective, we would expect the following interfaces:

interface LocaleContext {
    state: { locale?: string; };
}

interface I18N<T> {
    /* uses the ambient context to resolve the bundle */
    (bundle: T): Promise<T>;

    /* uses the context provided to resolve the bundle */
    (bundle: T, context: LocaleContext): Promise<T>;

    /* uses the specified locale to resolve the bundle */
    (bundle: T, locale: string): Promise<T>;
}

We also envisioned that from a consumer perspective, it would work something like this:

import messages from './bundles/messages';
import i18n from 'dojo-i18n/i18n';

const context = {
    state: {
        locale: 'fr'
    }
};

/* Promise with ambient locale */
i18n(messages)
    .then((bundle) => {
        bundle.greeting; // 'Hello'
    });

/* await with contextual locale */
const bundleFr = await i18n(messages, context);
bundleFr.greeting; // 'Bonjour'

/* await with asserted locale */
const bundleDe = await i18n(messages, 'de');
bundleDe.greeting; // 'Hallo'
mwistrand commented 8 years ago

In order for the loader to be able to retrieve the correct locale bundle, will the i18n function also need a path (or will the bundle itself need to register its own path)?

kitsonk commented 8 years ago

I think between i18n() or whatever is in the bundle should resolve the path to the specific locale.

mwistrand commented 8 years ago

So the bundle passed into i18n would need to have something like a bundlePath property? By the time the object is passed into the i18n function, it's just an object without any sort of context for where its locale-specific messages reside. One option is to expose a registry that bundles add themselves to with a module ID that the loader can resolve to the correct locale bundle:

import registerBundle from 'dojo-i18n/i18n';

const messages = registerBundle('nls/main', {
  hello: 'Hello',
  helloReply: 'Hello',
  goodbye: 'Goodbye'
});

export default messages;

Another might be to require a path on the i18n function (imo, not the best solution):

import messages from './bundles/messages';
import i18n from 'dojo-i18n/i18n';

i18n(messages, 'app/bundles/messages', 'de');
kitsonk commented 8 years ago

While this isn't a fully fleshed out thought, why wouldn't a bundle define that path... Something like this:

interfaces.d.ts

export interface I18NLabels {
    [label: string]: string | () => string;
}

export interface I18NBundle<L extends I18NLabels> {
    baseUrl: string;
    labels: L;
}

message.ts

import { I18NLabels, I18NBundle } from 'interfaces';

interface MessageLabels extends I18NLabels {
    greeting: string;
}

default export <I18NBundle<MessageLabels>> {
    baseUrl: './messages',
    labels: {
        greeting: 'Hello'
    }
};

i18n.ts

import { I18NLabels, I18NBundle } from 'interfaces';

function i18n<T extends I18NLabels>(bundle: I18NBundle<T>): Promise<T>;
function i18n<T extends I18NLabels>(bundle: I18NBundle<T>, context: LocaleContext): Promise<T>;
function i18n<T extends I18NLabels>(bundle: I18NBundle<T>, locale: string): Promise<T>;
function i18n(bundle: I18NBundle<any>, context: LocaleContext | string): Promise<T> {
    /* implementation */
}
mwistrand commented 8 years ago

That works. I was stuck on the idea that the bundle would be a map of keys to messages, but this is ultimately better.

kitsonk commented 8 years ago

Yeah, so i18n extracts the shape, and then mixins in whatever bundle is resolved and returns that in the promise. You will already have a default structure there in bundle.labels to mixin to!

kitsonk commented 8 years ago

It also helps if for some reason there is other meta data we have to end up supplying with the bundle... The beauty of generics, once you spend a year mind-melding with them.

mwistrand commented 8 years ago

The basic loading functionality is consolidated within the i18n module, exposing an i18n function as discussed. Please also take a look at the README to ensure the example using widgets makes sense.

kitsonk commented 8 years ago

I also enabled Travis on this repo... so any next push to this should cause the CI to kick off (which will also notify codecov.io)

agubler commented 8 years ago

Generally I think throughout we could use function shorthand (arrows) across the changes, especially for anonymous functions like callbacks.

There is an on going discussion on this topic that can be found https://github.com/dojo/meta/issues/45#issuecomment-252639229

agubler commented 8 years ago

@mwistrand I think this is good to go, however just need to resolve the file conflicts (.travis.yml) and we can merged.

kitsonk commented 8 years ago

Why is there i18n<Messages> all over the place now? Can't this be inferred contextually?

agubler commented 8 years ago

@kitsonk good point, I believe that they can be. As well as this there are lots of places that don't require explicit types (can be inferred from shape), unless there is a reason to keep them.

agubler commented 8 years ago

@mwistrand still needs conflicts fixing so it can be merged 😄

mwistrand commented 8 years ago

Git was not letting me rebase (git add .; git rebase --continue "there's nothing to add"), so I ended up having to cherry pick to make this work.

agubler commented 8 years ago

@mwistrand really strange that I could rebase from your branch 🎱

agubler commented 8 years ago

@mwistrand going to need to update ios to9.3 in intern-saucelabs.ts, 7.1 it seems has been removed.

codecov-io commented 8 years ago

Current coverage is 99.09% (diff: 99.09%)

No coverage report found for master at 3b180d0.

Powered by Codecov. Last update 3b180d0...f7da83b