-
Notifications
You must be signed in to change notification settings - Fork 167
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
[Feature Request] Replace app/utils/intl/missing-message.js
with hook
#1183
Comments
@jasonmit Does this sound like a good idea to you?
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! |
Yeah I'm in favor of this change.
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 ( Thoughts? |
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. |
@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 ( 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. 💯 |
FWIW I think that this change would also help make this compatible with
|
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 |
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. |
@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. |
app/utils/intl/missing-message.js
with hookapp/utils/intl/missing-message.js
with hook
The current behavior of ember-intl/addon/services/intl.d.ts Lines 63 to 74 in 62e0a0b
Because they can return I would propose that whatever API we come up with, enures that (at the very least) 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. |
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 At least in our private typings we made the return type |
I think we can have type |
How about replacing the magic file at
app/utils/intl/missing-message.js
with a hook on the service?Something like this:
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 #627The text was updated successfully, but these errors were encountered: