1095120 - Introduce asynchronous mozL10n.get

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We should introduce asynchronous replacement for mozL10n.get

L20n is leaning toward formatValue and formatEntity.

Comment 1

[Staś Małolepszy :stas]

Attachment: Add mozL10n.formatValue and mozL10n.formatEntity

https://github.com/mozilla-b2g/gaia/pull/26209
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=6283cc98e70f

Comment 2

[Staś Małolepszy :stas]

I made a small typo in one of the test files, new TH run:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=512d3fb8ad0d

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8524227 [details] [diff] [review]
Add mozL10n.formatValue and mozL10n.formatEntity

Review of attachment 8524227 [details] [diff] [review]:
-----------------------------------------------------------------

So I ended up rewriting the whole API... just kidding :)

I moved Resolver.formatValue and Resolver.formatEntity to Context as its private functions.  Since part of the whole fallback logic is in these methods, it made sense to move them out of the Resolver.  They both call Resolver.format internally.  (Come to think about it, maybe I should rename it to Resolver.resolve?)

This move allowed me to get rid of the whole return-null-or-undefined thing when errors occur and implement proper throwing.  Hence all the changes to test files.  The original idea was to avoid unnecessary try-catch blocks, but as soon as we introduced guards to prevent cyclic references, we added try-catches anyways, and ended up with a bit of a mess in the code.  The good thing is, the total number of try-catches remains unchanged.

Context.prototype.formatValue and Context.prototype.formatEntity are Context's public methods calling into their private counterparts, doing locale fallback on missing entries, enforcing asynchrony and cleaning up in case of errors.  They're exposed on mozL10n as mozL10n.formatValue and mozL10n.formatEntity, respectively.

I also changed how the legacy fallback approach works.  Before, mozL10n.get would return an empty string if no translation was found or when the first found translation was broken.  Now, it will only return an empty string if no translation is found.  However, if a broken translation is found, the requested id will be returned.  This way the code which relies on checking the return value to decide whether a translation exists can continue to work as it does right now. And we can continue cleaning it up in bug 1020138 while we slowly introduce proper L20n mechanics.

This legacy fallback-on-empty-string scheme is not used by mozL10n.formatValue nor mozL10n.formatEntity.  Both always use fallback-on-id.

I also changed how we react to broken indexes.  Before, we'd try to find an 'other' key and return it.  In the patch I throw immediately.  I'm hoping this will help discover errors earlier, but I don't feel strongly about this.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8524227 [details] [diff] [review]
Add mozL10n.formatValue and mozL10n.formatEntity

Review of attachment 8524227 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/resolver.js
@@ +59,5 @@
>    };
>  }
>  
>  
> +function format(args, entity) {

why are you putting args as the first argument everywhere? I feel like it inflates this patch and makes it harder to review.

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8524227 [details] [diff] [review]
Add mozL10n.formatValue and mozL10n.formatEntity

Review of attachment 8524227 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/resolver.js
@@ +59,5 @@
>    };
>  }
>  
>  
> +function format(args, entity) {

I did that for consistency with other internal functions in resolver.js (which take args and env first, entity second) and to make it easier to bind args in Context::formatAsync.  There's little semantic rationale behind this other than a convenience one.  It's an internal function anyways.

Comment 6

[Staś Małolepszy :stas]

Zibi, you can follow along my commits here, which should make the review easier:

https://github.com/stasm/l20n.js/compare/1095120-async-format

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8524227 [details] [diff] [review]
Add mozL10n.formatValue and mozL10n.formatEntity

Review of attachment 8524227 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/context.js
@@ +66,5 @@
> +
> +function formatEntity(args, entity) {
> +  if (!entity.attrs) {
> +    return {
> +      value: formatValue.call(this, args, entity)

add attrs: null here, so that the shape of the object stays the same.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8524227 [details] [diff] [review]
Add mozL10n.formatValue and mozL10n.formatEntity

Review of attachment 8524227 [details] [diff] [review]:
-----------------------------------------------------------------

r+, perf looks good.

Comment 9

[GitHub Autolander Bot]

Attachment: [PullReq] stasm:1095120-async-format to mozilla-b2g:master

Comment 10

[Staś Małolepszy :stas]

Merged into l20n.js master: https://github.com/l20n/l20n.js/commit/0540b6e37c46fcad55641a4b5421286163b0b655

Comment 11

[Staś Małolepszy :stas]

I had to land a follow up:

https://github.com/l20n/l20n.js/commit/90c20bc5b5afbb97177bb81191570406804f555b

I'm not really happy with this though.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/a9b4ea3a85b3907525fba3b289f4c758465ca6ff