Introduce asynchronous mozL10n.get

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gandalf, Assigned: stas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
We should introduce asynchronous replacement for mozL10n.get

L20n is leaning toward formatValue and formatEntity.
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
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.
Attachment #8524227 - Flags: review?(gandalf)
(Reporter)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

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

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

Comment 7

4 years ago
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.
(Reporter)

Comment 8

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

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

r+, perf looks good.
Attachment #8524227 - Flags: review?(gandalf) → review+
Created attachment 8532322 [details] [review]
[PullReq] stasm:1095120-async-format to mozilla-b2g:master
(Assignee)

Comment 11

4 years ago
I had to land a follow up:

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

I'm not really happy with this though.
(Reporter)

Updated

4 years ago
Attachment #8532322 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.