Bring back View.formatValue

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We consolidated formatEntity and formatValue, but it seems that all use cases for format() in Gaia currently want just formatValue.

I believe it would be worth bringing it back to simplify the API usage and reduce the amount of code run when just the value is needed.
(Assignee)

Comment 1

3 years ago
Created attachment 8649522 [details] [review]
pull request

Use case:

All calls to format here: https://github.com/mozilla-b2g/gaia/compare/master...zbraniecki:settings-l20n
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8649522 - Flags: review?(stas)
The code looks great, but I'm still not convinced it's the right thing to do.  Let me explain.

It feels to me that we're adding new code (18 lines of it and likely more due to the | return { value: id, attrs: null }; | problem I commented on in the PR) and more importantly, a new API method which we'll need to maintain just to cater to a very specific and non-perf-critical use-case.

Looking at the code you pointed at in comment 1, format() is often used in combination with window.alert or window.confirm.  By definition, we don't need attributes in these entities, so in fact, format() will not do much extra work here:  the attrs should not be present.  If the developer can figure out to use formatValue instead of format, we should assume they can also figure out that the translation that goes into a window.alert doesn't need attributes.

So the entire additional cost that we're paying in format() in the use-cases you pointed at is:

 - the object created here: https://github.com/l20n/l20n.js/blob/1e0d69965460a8bb94aabe59dcbb3b0a08b2daa3/src/lib/context.js#L27-L30 which will be instantly GC'ed if you only use the value, and

 - the if condition here: https://github.com/l20n/l20n.js/blob/1e0d69965460a8bb94aabe59dcbb3b0a08b2daa3/src/lib/context.js#L32

That's tiny given that it's run on non-start-up or not even in CPU-intense tasks.  It's run just before a window.alert which halts everything and waits for the user to interact.

This teeny-tiny cost gives us a cleaner API with one method fewer, which is easier for developers because they don't need to remember that there's this other method and wonder which one they need.  One format() to rule them all.

I'm trying to balance out the complexity of the API with the use-case we want to cater to.  Looking at the examples you provided, I think we should stick to a single format() method.
(Assignee)

Comment 3

3 years ago
(In reply to Staś Małolepszy :stas from comment #2)

> It feels to me that we're adding (...) a new API method which we'll need to maintain
> just to cater to a very specific and non-perf-critical use-case.

> This teeny-tiny cost gives us a cleaner API with one method fewer, which is
> easier for developers because they don't need to remember that there's this
> other method and wonder which one they need.  One format() to rule them all.

(...)
 
> I'm trying to balance out the complexity of the API with the use-case we
> want to cater to.  Looking at the examples you provided, I think we should
> stick to a single format() method.

I agree with you that it's a topic of clean API, but I believe that you are looking at it from the perspective of the inside details of our library and aims at keeping the internals of the library lean at the cost of the users code.

document.l10n.format(l10nId).then(({value}) => fn(value));

vs.

document.l10n.formatValue(l10nId).then(fn);

and that's just one example, since as we move to more complex scenarios with Promise.all, we'll have to do the same for multiple entity objects instead of getting values.


And I would be even ok with that if there was at least 20/80 split between use cases for format and formatValue. But so far, working on preparing Gaia apps for V3 it feels to me that there is absolutely zero use cases for document.l10n.format. It's useless and can be fully replaced with formatValue.

So let me try this. How about we add formatValue and remove format. It does nothing and is not useful for our DOM API consumers, so maybe we should not have it at all unless we find a use case.*

This way we could promote the concept that entities with attributes are for HTML entities localization while entities with only values are the ones to be used for non-HTML targets.

*) The only use case I can imagine right now is re-use. An entity used in HTML has an attribute that we want to take as a value and use somewhere else - which, I believe deserves two entities because the will appear in different pieces of UI.
Flags: needinfo?(stas)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)

> document.l10n.format(l10nId).then(({value}) => fn(value));
> 
> vs.
> 
> document.l10n.formatValue(l10nId).then(fn);

That's not an entirely fair comparison.  The format() version can also look like this:

  document.l10n.format(l10nId).then(fn);

It's up to the consumer (fn) to decide if it unpacks the entity or not.

> And I would be even ok with that if there was at least 20/80 split between
> use cases for format and formatValue. But so far, working on preparing Gaia
> apps for V3 it feels to me that there is absolutely zero use cases for
> document.l10n.format. It's useless and can be fully replaced with
> formatValue.

Can there be attributes that are not used in HTML but in JS?  Can there be any other meta-data that the localizer would want to translate that would be a public attribute?  What if in the future we decide to add Number-typed attributes and we'll want localizers to provide the width of the alert dialog? Etc.

Also, what if we wanted to start returning the language in which the entity was resolved? Or anything else that is not the value.

I don't know if anything like that will happen, but I feel like replacing format with formatValue limits our options in the future.

> So let me try this. How about we add formatValue and remove format. It does
> nothing and is not useful for our DOM API consumers, so maybe we should not
> have it at all unless we find a use case.*
> 
> This way we could promote the concept that entities with attributes are for
> HTML entities localization while entities with only values are the ones to
> be used for non-HTML targets.

OK, I like the idea of having one method here.  What I'm afraid in the scenario you proposed is that we're going to remove format, add formatValue, and the on the first occasion when it turns out that we actually need to return more than the string value, we'll re-add format() and we'll end up with two methods.  Because how else?  And bam!, our API has grown.

format() does everything you want and is completely forward-compatible.  All the use-cases that you've seen are in non-startup code where perf doesn't matter.  What's wrong with it?
Flags: needinfo?(stas)
(Assignee)

Comment 5

3 years ago
(In reply to Staś Małolepszy :stas from comment #4)
>  What's wrong with it?

It's an API that's designed to keep our internals lean instead of to cater to the needs of the API users. I believe that we should design our API's in reverse of that.

If users need formatValue and don't need format, then giving them format and not formatValue is a design mistake from my perspective.
(Assignee)

Updated

3 years ago
Flags: needinfo?(l10n)

Comment 6

3 years ago
I think that entity attributes are something that's rather unique to DOM localization.

In general, I think that developers getting strings programmatically will follow the original concept of getting just a string, independent of semantics.

I think that other language bindings for other frameworks might have a use case for attributed localizations, but I expect those to be part of the language binding.

L20n.js is a tad special here, as we're implementing the html binding with the js binding, and thus need a bit more detail that programmers in general.

Thus, my recommendation would be to go with formatValue as the default API.
Flags: needinfo?(l10n)
Outnumbered, I concede :)

Let's remove format() entirely and replace it with formatValue().
(Assignee)

Comment 8

3 years ago
nail in the coffin is that in Settings scenario census shows that this is one of the top three allocation locations: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l20n.js#L2099-L2102
Which we can't remove because the HTML bindings still need this, right?
(Assignee)

Comment 10

3 years ago
Possibly yes, I don't know yet.

But even if we do, that still means that we can skip this for all formatValue calls which are common in Settings.
(Assignee)

Comment 11

3 years ago
Hey Stas, updated the patch.

Can you look at it?
Comment on attachment 8649522 [details] [review]
pull request

Can you also add a few test for ctx.resolveValue, especially for the missing translation case?

I also wonder if we should hide view.resolve from the api somehow.  Maybe also rename it to resolveEntity for consistency?

r=me with these comments, thanks.
Attachment #8649522 - Flags: review?(stas) → review+
Blocks: 1202442
Comment on attachment 8649522 [details] [review]
pull request

Sorry, this is actually an r- because it's missing the necessary changes to src/bindings/gaiabuild. Should be easy to fix though.
Attachment #8649522 - Flags: review+ → review-
(Assignee)

Comment 14

3 years ago
Comment on attachment 8649522 [details] [review]
pull request

I added some tests and renamed view.resolve to view._resolveEntity.

I did not make it private because that would require a bit more mingling in our dependencies and I didn't want to extend the size of this patch.

We can look into this later and for now, the name starting with "_" should indicate that it's not public.
Attachment #8649522 - Flags: review- → review?(stas)
Comment on attachment 8649522 [details] [review]
pull request

lgtm, thanks.
Attachment #8649522 - Flags: review?(stas) → review+
(Assignee)

Comment 16

3 years ago
Commit: https://github.com/l20n/l20n.js/commit/5a404763fed1dd7fe54ba1fb37a8068bbc363f0d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.