[Feature Request] Replace app/utils/intl/missing-message.js with hook

[sandstrom]

How about replacing the magic file at app/utils/intl/missing-message.js with a hook on the service?

Something like this:

// ember-intl service
// …
missingTranslation(locale, key, options = {}) {
  return `Missing translation for ${key} in ${locale}`;
}

// …

That hook could be used both to modify the returned message, e.g. by hooking up a fallback message in a specific locale, or used to notify some backend system about the missing translation.

---

The missing-message magic file works, but it's an unorthodox way of doing it, and it causes some confusion, for example #627

[sandstrom]

@jasonmit Does this sound like a good idea to you?

• Do you have any preferences on the method signature for such a hook?

• For the third argument options, to we want to pass along the options that was originally passed into the t helper?

• In what cases should we trigger this? Only for t helper, t computed property and t on service?

• Should it also trigger for lookup? I'm thinking it's better not to.

Would it be enough to motivate deprecating the missing-message magic file in 5.x?

I strongly think we should deprecate it, seems like that was a legacy thing brought over from ember-i18n. I can't figure out a case where it would be better than a hook. But please let me know if I've missed something obvious!

[jasonmit]

Yeah I'm in favor of this change.

In what cases should we trigger this? Only for t helper, t computed property and t on service?
Should it also trigger for lookup? I'm thinking it's better not to.

I prefer the behavior of notifying even on failed lookup via console warnings.. This way it's very clear when something has gone wrong.

We should provide this context to the function, perhaps as a third argument (kind | type). This way if users want to implement this hook to do nothing on failed lookups they can or optionally throw on failed lookups originating from intl.t() or {{t}}

Thoughts?

[jasonmit]

I started on a generic onError hook that will pass in a type and the error - will be used for missing translations and missing polyfills for now. It's not yet public but will likely make it public before 5.0.0.

[sandstrom]

@jasonmit Awesome! 🎉

Good point, if we can give people something to switch on (context), notifying on failed lookups makes sense too (that way users can choose how to handle it).

Passing an extra argument (kind/type) is certainly one way and I don't object to it.

But perhaps encoding that in the error itself would also work?

For example:

// ALT 1

import { MISSING_TRANSLATION } from "…/…/…";
// …
onError(err) {
  if (err instanceof MISSING_TRANSLATION)
    // do something
  } else if (err instanceof SOME_OTHER_TYPE) {
    // …
  }
}

// ALT 2

onError(err) {
  if (err.type === 'missingTranslation')
    // do something
  } else if (err.type === 'someOtherErrorTypeCouldBeAnything')
    // …
  }
}

Just wanted to put the idea out there. Either way, I a public onError hook to replace the missing-message magic file would be a great addition. 💯

[jacobq]

FWIW I think that this change would also help make this compatible with ember-cli-typescript, i.e. prevent a message like the following (typed-ember/ember-cli-typescript#1046) when trying to generate missing-message.js from missing-message.ts

WARNING: Detected collisions between .js and .ts files of the same name. This can result in nondeterministic build output; see https://git.io/JvIwo for more information.
  - my-app/utils/intl/missing-message.{js,ts}

[sandstrom]

Let's continue the discussion in #1299 (comment) here.

I think having hooks that could throw errors, return a custom string, or do any work really, would very useful.

The default behaviour could be to raise exceptions, for example for missing translations, missing args, etc. But hook would allow customization. That way one can choose to alter behaviour depending on environment, return a string + notify Sentry/TrackJS/Bugsnag, send notification to backend, etc.

Our upgrade to ember-intl has been smooth overall, but one issue that has cropped up several times is production crashes on missing translations or arguments (e.g. unresolved promise). Obviously those are bugs in our code, but with the old lib we'd just render fallback strings or error messages and our app would still be useful (+ send notification behind the scenes). Now everything just crashes.

Hooks where we could customize behaviour upon failure would be very useful!

Thanks again for all your awesome work on this addon! 🏅

@jasonmit let me know what you think!

cc @jacobq

[jasonmit]

I do still plan to implement this just my time has been spent bug fixing and improving other parts of the lib. I'll land this eventually - hopefully within the next few weeks.

[sandstrom]

@jasonmit I don't mean to rush you!

I just wanted to connect the issues @jacobq mentioned in the issue he opened back to this one, since I think the hooks discussed here could give him adjustability around how errors are handled, and instead of having to choose between fail-hard vs. soft-fail, a hook would let the user choose whether to return a value or raise depending on preference/environment/etc.

[buschtoens]

The current behavior of missingMessage also happens to make the types for t and lookup über-annoying. 😄

ember-intl/addon/services/intl.d.ts

Lines 63 to 74 in 62e0a0b

	lookup(key: string, localeName?: string | string[]): string | undefined;
	lookupAst(
	key: string,
	localeName?: string | string[],
	options?: { resilient: boolean }
	): MessageFormatElement[] | undefined;
	private validateKeys(keys: string[]): void;
	private validateKeys(keys: unknown[]): void | never;
	t(key: string, options?: TOptions): string | undefined;

Because they can return undefined, code that would just expect t to return a string, as it should normally, needs to either special-handle the potential undefined or use the non-null assertion operator. This is also true for the t test helper, which means you need to insert the non-null assertion operator, even after using the setupIntl(hooks) test helper.

I would propose that whatever API we come up with, enures that (at the very least) t always returns a string, by default.

We could for instance have:

t(key: string, options?: TOptions): string | never; // throw on missing translation

t(key: string, options?: TOptions): string | ''; // return an empty string on missing translation

Alternatively, we could make the type dependent on the return value of another method, e.g. missingMessage, which by default could throw an error / return an empty string.

[buschtoens]

Sorry for hijacking this thread with my TypeScript escapades, I can happily branch off into a different thread, if you like.

I'm contemplating whether we should release v5.5.0 with the string | undefined return type for t at all – I think it may introduce a lot of unintentional breakage for TypeScript users. The reason being that most TS users will either type the service injection as any and // @ts-ignore the test support imports, which effectively makes them any as well, or they'll have custom typings (like we do at @ClarkSource).

At least in our private typings we made the return type string, because it grows insanely annoying otherwise.

[jasonmit]

I think we can have type t() to always return a string. The default missing message util already returns a string so I don't know of a scenario where it would return undefined unless someone implemented their custom missing messages fn to do that.