1204086 - Add formatValues API

Status: RESOLVED FIXED

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

Description

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

One of the areas we didn't cover fully while working on v3 API is a scenario in which the developer needs to get many entities at once in order to operate on them.

Good examples are loading list of city names to be sorted and displayed.

With Intl.Collator we have a fast way to sort the list, but what's left is how to resolve high number of of entities at once.

The only pattern that we offered so far is based on Promise.all. It works more or less like this:

--------------------------

var keys = [
  'key1',
  'key2',
];

var promises = [];

for (var i = 0; i < keys.length; i++) {
  promises.push(document.l10n.formatValue(keys[i]));
}

Promise.all(promises).then(vals => {
  for (var i = 0; i < vals.length; i++) {
    console.log(keys[i] + ': ' + vals[i]);
  }
});


--------------------------

My hypothesis was that for default case (where all entities are available in the first language) this approach will be significantly slower then if we could get all entities at once, so I wrote a simple POC that adds document.l10n.formatManyValues API.

The way it works is that it mimics formatValue from View, via Service down to Context. Inside context it loops over ids using ctx._getEntity and ctx.formatValue and returns an object with {id: val}.

The performance on desktop for ~800 entities is:

1) formatValue+Promise.all: 60ms
2) formatManyValues: 3ms

I think it warrants a conversation about how we should approach the scenario and revisiting something else then Promise.all for that case.

Comment 1

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

Attachment: pull request

I'm not happy with the code in Context for that. But the performance gains are significant, so I'd like to start a conversation about it.

Comment 2

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

Stas, Axel, what do you think?

Comment 3

[Axel Hecht [:Pike]]

I sympathize with the intent, but I don't understand the code at all.

Not sure if I agree that the requested keys should be passed in as Object, or rather as Array. Also, I'm torn if I'd allow args to be passed in.

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8660093 [details] [review]
pull request

I left a few comments in the PR:  it looks like resolveManyValues never actually returns 'ret'?  I tried to run that code locally and it broke because _resolveManyValues was not defined.

I'd like to see the perf numbers with the code fixed, please :)  I monkey-patched your branch to just return ret at the end of resolveManyValues (before the fallback) code, and I got 5ms but if you could have another look, that would be great.

Like Axel, I also sympathize with the idea -- perhaps we could start by only using this API internally to make localization via the mutation observer faster?

I don't think ret should be an object;  probably an array or a Map of WeakMaps?  I'd like to be able to pass different args to the same entity.

I'm also not sure how the fallback should work.  Your code doesn't actually tackle this at all:  what happens when one of the requested entities is missing in the current language?  Do we fallback just for this one entity?  Or do we assume that the requested batch is somehow related and we should prioritize the consistency of all translations and fall back for all of them?

Let's continue discussing this.  If the perf is good, I think it might be worth introducing this internally first.  Can you also describe the city names use-case in detail?  Is that something we'd need the document.l10n API for, or can we rely on DOM localization here as well?

Comment 5

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

Cool! Thanks for the feedback guys!

I mainly wanted to see what is the sentiment toward such function in the light of the performance numbers I got.

I'll update the PR and we'll move from there.

Comment 6

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

I updated the PR to address the feedback and handle fallback.

I also tested performance on the Flame device using the same test. The numbers:

1) for Promise.all - 665ms
2) for formatValues - 36ms

That still feels like a huge difference and I also start to suspect that it may impact memory as well.

I understand that those are extreme conditions (Settings app is loading ~200 entities at the startup, not 850), but I believe that the performance impact warrants considering this solution.

My current thinking is:

1) clean up the code. I believe that there's a room to make ctx.resolveValues look cleaner
2) _resolveEntity is an internal API, replace it with resolveEntities that uses this approach
3) consider modifying formatValue to either accept an array or rename it to formatValues

I'm torn wrt. 3) because on one hand, most often we want to format a single entity and we want to pass the value so it would be really nice not to have to keep an intermediate function to get the first value out of the returned one-element Array.

Maybe we could just make formatValue a wrapper on formatValues?

Stas, what do you think? I'm a little bit overwhelmed with the internals of ctx.resolveValues and could use your help to make it look decent :)

Comment 7

[Staś Małolepszy :stas]

I like what you have so far although I'm not sold on polymorphism here.  I'm not sure I like method overloading in JavaScript, but I might come around in the future :)

I've been thinking about the Context and the View API here.  The main difference between the two is that Context methods are fast whereas View methods are safe.  What I mean by that is that the View is responsible for waiting on languages and ctx.fetch before calling any of the Context methods directly.  This makes it possible for the view to wait once and then query the context multiple times.

If getting multiple values from context is the default, then we could perhaps move the fetch step (currently forced by the view) back into context. This would simplify the logic of the view code and the relationship between view <-> service <-> context for the view.  I thikn we should do this in this bug.

This new ctx.resolveEntities should be our work-horse for the DOM bindings.  translateMutations, translateDocument, translateFragment—you name it.  Everything should use it and benefit from the speed improvement.

For the JS API, in bug 1196009 we decided to go with the value-returning method.  For this, we can't use ctx.resolveEntities; we need another API.  If I understand your proposal right, we'd need to:

 - introduce ctx.resolveValues with a similar logic like the one in ctx.resolveEntities, but using a different format function (formatValue instead of formatEntity),

 - add view.formatValues which calls ctx.resolveValues,

 - add view.formatValue which is a wrapper around view.formatValues.

That might make sense, but I still haven't seen a good use-case for view.formatValues.  You mentioned it in bug 1204152 comment 1, but without any examples.  Depending on the use-case and how rare/frequent it is, it might make sense to only have the singular view.formatValue, or expose view.formatEntities for the plural case.

Comment 8

[Staś Małolepszy :stas]

I was also thinking about the arguments this API should take.  In it's most basic form, we could accept an iterable of arrays:

  document.l10n.formatValues([
    ['welcome'],
    ['hello', { name: 'Mary' }]
  ]);
  
  // -> Promise resolving to:
  // ['Welcome to The App', 'Hi there, Mary!']

The common case of formatting one value would then look like this:

  document.l10n.formatValues([
    ['hello']
  ]);
  document.l10n.formatValues([
    ['hello', { name: 'Mary' }]
  ]);

If introduce, the formatValue wrapper would make this a bit cleaner:

  document.l10n.formatValue('hello');
  document.l10n.formatValue('hello', { name: 'Mary' });
  
  // -> Promise resolving to:
  // 'Hi there, Mary!'

* * *

An alternative which could make the singular formatValue not needed could be to accept varying numbers of arguments:

  document.l10n.formatValues(['hello']);
  document.l10n.formatValues(['hello', { name: "Mary" }]);
  document.l10n.formatValues(['welcome'], ['hello', { name: 'Mary' }]);
  
  // -> Promise resolving to an array of strings

This one is simpler but a little bit limiting.  Would we ever want to pass an additional args object to be used for all requested translations?  Like so:

  document.l10n.formatValues(['hello', 'welcome'], { name: 'Mary' });

Again, it would help to better understand what we need this API for.  If it's for building an email-list UI where messages come from different people and we're using the same string with different args, then we need DOM elements anyways and perhaps DOM bindings are a better choice.  If it's for building a list of cities or timezones, then perhaps we shouldn't stress about args-per-translations so much.

Thoughts?

Comment 9

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

My perspective is that we don't want to have same args for multiple entities and if there will be an exception, they can construct an array.

Secondly, I don't think I have an opinion on formatValues(...idArgs) vs formatValues(idArgs). I asked on #jsapi and they said that if we'd want to not have to interpret all idArgs elements then the latter would be better, because spread will interpret all elements, and that the engine may do a little bit smarter things with an array when it comes to types than with arguments list, but that shouldn't be a deciding factor.

The only thing I do have an opinion on is handling l10nId vs. [l10nId, l10nArgs]. I really believe that it's worth our time and effort to make the API prettier for our consumers. Since I believe that most of the code that will only need ids, and at the same time there will be code that needs args, I believe it's worth the cost of two isArray to make it work like this:

document.l10n.formatValues('hello', 'world').then(([helloString, worldString]) => {});

and

document.l10n.formatValues('hello', ['welcome', {name: 'Mary'}]).then(([helloString, welcomeString]) => {});

> That might make sense, but I still haven't seen a good use-case for view.formatValues

Sure, I have two examples:

1) https://bugzilla.mozilla.org/show_bug.cgi?id=1204716#c4

In this example, I'd love instead of:

Promise.all([
  navigator.mozL10n.formatValue('unknownTitle'),
  navigator.mozL10n.formatValue('unknownArtist'),
  navigator.mozL10n.formatValue('unknownAlbum')
]).then(([unknownTitle, unknownArtist, unknownAlbum]) => {
});

do:

document.l10n.formatValues('unknownTitle', 'unknownArtist', 'unknownAlbum']).then(
  ([unknwonTitle, unknownArtist, unknownAlbum]) => {}
);

2) https://github.com/mozilla-b2g/gaia/blob/750334a998ffc4b2d29fbd04e394d8cdd14fd29c/shared/js/tz_select.js#L130-L142

Here, I'd probably like to do:

var tzIds = gTZ.map(c => 'tzRegion-' + c);

document.l10n.formatValues(...tzIds).then(tzStrings => {
  var options = [];
  for (i=0; i < gTZ.length; i++) {
    options.push({
      text: tzStrings[i],
      value: gTZ[i],
      selected: (gTZ[i] === gRegion)
    });
  }
});

Comment 10

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

Comment on attachment 8660093 [details] [review]
pull request

I tested this patch against my settings-l20n branch.

On default locale there is basically no impact on perf or mem.

On non-default, on Flame, there seem to be 50+ ms win in perf and 70kb win in memory usage!

I'll work on tests tomorrow if you like the code :)

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8660093 [details] [review]
pull request

I like where this is going, thanks.

 1) Can we somehow clean up examples/web.js?  I understand you use it now to test performance, but maybe it should go into a special directory?  examples/tests or something.  I could see us adding more test cases in there in the future:  memory consumption, postMessage experiments and others.

 2) What's the final decision on the signature of view.formatValues?  In comments you suggested ...args but in the PR it's explicitly an array.  My vote goes to ...args.

 3) On the return value (to which the promise resolves), I've been thinking about using a Map.  It works great for primitives, and encourages thinking about the args list beforehand if you need to interpolate strings.  You can also just iterate over it with forEach(), entries() or values().

Example 1:

  document.l10n.formatValues(
    'unknownTitle', 'unknownArtist', 'unknownAlbum']).then(
      translationMap => console.log(translationMap.get('unknownTitle)));

Example 2:

  const tzIds = gTZ.map(c => 'tzRegion-' + c);

  document.l10n.formatValues(...tzIds).then(translationMap => {
    var options = [];
    translationMap.forEach((val, key) => {
      options.push({
        text: val,
        value: key,
        selected: (key === gRegion)
      });
    }
    // do something with options
  });

With an array, the best you can do is iterate or access elements by index.  With a Map, you can also iterate, but for simple strings it also gives you an easy way to access translations by id (via map.get(id)).  What do you think?
  

 4) I mentioned earlier that this PR will make it possible to move the ctx.fetch guard back into the resolve* methods.  At first I suggested we do it in this bug, but on second thought, let's do it in a follow-up.

Comment 12

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

(In reply to Staś Małolepszy :stas from comment #11)
> Comment on attachment 8660093 [details] [review]
> pull request
> 
> I like where this is going, thanks.
 
>  2) What's the final decision on the signature of view.formatValues?  In
> comments you suggested ...args but in the PR it's explicitly an array.  My
> vote goes to ...args.

I decided to go for ...args because:

document.l10n.formatValues('foo1', 'foo2') looks better than document.l10n.formatValues(['foo1', 'foo2'])

while

document.l10n.formatValues(keys) vs. document.l10n.formatValues(...keys) seems comparable.

>  3) On the return value (to which the promise resolves), I've been thinking
> about using a Map.  It works great for primitives, and encourages thinking
> about the args list beforehand if you need to interpolate strings.  You can
> also just iterate over it with forEach(), entries() or values().

I like some aspects of your proposal, but unfortunately there are two concerns:


1) our dom.js chain of translateElements -> getElementsTranslations -> applyTranslations does not pass keys. It passes elems and translations which nicely fits with indexed list.

I can imagine either passing keys as another argument or transforming elems to be a Map with the same keys, but it seems like a superfluous solution.
I'd be down for that, if only customer code really looked that much better.

2) WHile I like the map approach, there's one problem. One can't recreate object keys:

document.l10n.formValues(
  'foo1',
  {id: 'foo2', {name: 'John'}},
  'foo3'
).then(translationMap => {
  console.log(translationMap.get('foo1')); // works
  console.log(translationMap.get({id: 'foo2', {name: 'John'}})); // doesn't work
  console.log(translationMap.get('foo3')); // works
});

Compare it to:

document.l10n.formValues(
  'foo1',
  {id: 'foo2', {name: 'John'}},
  'foo2'
).then(([foo1, foo2, foo3]) => {
  console.log(foo1);
  console.log(foo2);
  console.log(foo3);
});

I feel like this will be a scenario common enough that the added complexity of the Map is going to impact the readability of the code.

For that reason I'd vote to stick to Array.

Comment 13

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

Comment on attachment 8660093 [details] [review]
pull request

Ok, I fixed tests and lint.

I believe it's ready for review. There are couple minor things left like:

1) I'd like to add tests specific to resolveValues and resolveEntities to basic_test.js - mostly to test if multiple values including missing ones will return the right things.

2) I'd like to consider placing the example/web.html test files that I used somewhere in the scope to keep the performance of formatValues measured. I may move them out to perf tests.

3) I'd like to document this code a bit.

But I believe it's ready for review since the remaining three tasks are not part of the build and you can review them post-landing, but I'd like to make sure that you agree with me on the approach - especially wrt. Map vs. Array, and you like the code of the patch.

Comment 14

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> I decided to go for ...args because:
> 
> document.l10n.formatValues('foo1', 'foo2') looks better than
> document.l10n.formatValues(['foo1', 'foo2'])
> 
> while
> 
> document.l10n.formatValues(keys) vs. document.l10n.formatValues(...keys)
> seems comparable.

Cool.

> 1) our dom.js chain of translateElements -> getElementsTranslations ->
> applyTranslations does not pass keys. It passes elems and translations which
> nicely fits with indexed list.
> 
> I can imagine either passing keys as another argument or transforming elems
> to be a Map with the same keys, but it seems like a superfluous solution.
> I'd be down for that, if only customer code really looked that much better.

I don't think we'd need to pass keys.  A Map iterates its elements in insertion order, so instead of this:

  function applyTranslations(view, elems, translations) {
    for (let i = 0; i < elems.length; i++) {
      overlayElement(elems[i], translations[i]);
    }
  }

you can simply do:
   
  function applyTranslations(view, elems, translations) {
    translations.forEach(trans => overlayElement(elems.shift(), trans));
  }


> 2) WHile I like the map approach, there's one problem. One can't recreate
> object keys:

Well, yeah :)  I thought we talked about this.  That's why I asked for use-cases and example code.  See my comment 11.  For simple cases using primitive keys, the maps gives you the get() method which is nicer than an indexed array.  For complex cases, you're either better off iterating over the translations or you can bind the args object to a variable, or use destructuring to your advantage.

The way I'd recommend implementing your example this is:

  const keys = ['foo1', 'foo2', 'foo3'].map(
    key => [key, {name: 'John'}]);

  document.l10n.formatValues(keys).then(
    translationMap => translationMap.forEach(
      trans => console.log(trans)));

or, if you really need the specific key:

  document.l10n.formatValues(keys).then(
    translationMap => console.log(translationMap.get(keys[1])));

or use destructuring:

  document.l10n.formatValues(
    'foo1', ['foo2', {name: 'John'}], 'foo3'
  ).then(
    ([foo1, foo2, foo3]) => console.log(foo2[1]));

or:

  document.l10n.formatValues(
    'foo1', ['foo2', {name: 'John'}], 'foo3'
  ).then(
    translationMap => {
      const [foo1, foo2, foo3] = translationMap.values();
      console.log(foo2);
    });

> I feel like this will be a scenario common enough that the added complexity
> of the Map is going to impact the readability of the code.

Well, arguably none of the use-cases we discussed so far wanted to pass args like this.  But even if it turns out to be more common than we've seen so far, I hope to have demonstrated that a Map gives us all the an array does, plus much more.

What do you think?

Comment 15

[Staś Małolepszy :stas]

Comment on attachment 8660093 [details] [review]
pull request

Canceling the review request to give us a chance to discuss Array vs. Map.  Otherwise this look good.  Please also remember to edit _resolveEntity in src/bindings/gaiabuild/view.js.

Comment 16

[Axel Hecht [:Pike]]

tidbit, I guess the array API allows to localize multiple strings with the same ID and different data?

Comment 17

[Staś Małolepszy :stas]

Map allows this as well:  keys can be (id, args) tuples, with id being the same and args changing for each entry.

Comment 18

[Axel Hecht [:Pike]]

Recreated gandalf's concern, you can't recreate it;

foo = Map()
foo.set(['hi',{'arg':5}], 'something')
foo.get(['hi',{'arg':5}])
> undefined
foo.get(foo.keys().next().value)
> "something"

Comment 19

[Staś Małolepszy :stas]

But that's not how this API will be used. If you're passing in the same string with different args, you're most likely going to iterate over the return values anyways, for which the Map gives you the forEach method and a number of iterators.

Comment 20

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

(In reply to Staś Małolepszy :stas from comment #19)
> But that's not how this API will be used. If you're passing in the same
> string with different args, you're most likely going to iterate over the
> return values anyways, for which the Map gives you the forEach method and a
> number of iterators.

The thing is that I believe that there are two scenarios here:

1) Iterate over an entity with different value for an arg:

const l10nKeys = [];
for (let i = 0; i < 10; i++) {
  l10nKeys.push(['itemsCount', {n: i}]);
}

// 1:
document.l10n.formatValues(...l10nKeys).then((translations) => {
  for (let i = 0; i < 10; i++) {
    console.log(translations[i]);
  };
});

// 2:
document.l10n.formatValues(...l10nKeys).then((translations) => {
  l10nKeys.forEach(key => {
    console.log(translations[key]);
  });
});

2) Pass a an attribute to a known value:

// 1:
document.l10n.formatValues(
  'unknownTitle',
  ['unknownCount', {'count': db.unknownCount}],
  'unknownArtist',
  'unknownSong').then((unknownTitle, unknownCount, unknownArtist, unknownSong) => {
    db.title = unknownTitle;
    db.count = unknownCount;
    db.artist = unknownArtist;
    db.song = unknownSong;
});

// 2:

const l10nKeys = [
  'unknownTitle',
  ['unknownCount', {'count': db.unknownCount}],
  'unknownArtist',
  'unknownSong'
];

document.l10n.formatValues(...l10nKeys).then((translations) => {
  db.title = translations[0];
  db.count = translations[1];
  db.artist = translations[2];
  db.song = translations[3];
});

And the latter is a more common case from my experience. That's why I vote for an Array.

Comment 21

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

Ugh, in the last example, I meant:


const l10nKeys = [
  'unknownTitle',
  ['unknownCount', {'count': db.unknownCount}],
  'unknownArtist',
  'unknownSong'
];

document.l10n.formatValues(...l10nKeys).then((translations) => {
  db.title = translations.get(l10nKeys[0]);
  db.count = translations.get(l10nKeys[1]);
  db.artist = translations.get(l10nKeys[2]);
  db.song = translations.get(l10nKeys[3]);
});

Comment 22

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)

> The thing is that I believe that there are two scenarios here:
> 
> 1) Iterate over an entity with different value for an arg:

> // 2:
> document.l10n.formatValues(...l10nKeys).then((translations) => {
>   l10nKeys.forEach(key => {
>     console.log(translations[key]);
>   });
> });

Even better:

  document.l10n.formatValues(...l10nKeys).then(translations => {
    translations.forEach((key, trans) => {
      console.log(trans);
    });
  });


> 2) Pass a an attribute to a known value:
> 
> // 1:
> document.l10n.formatValues(
>   'unknownTitle',
>   ['unknownCount', {'count': db.unknownCount}],
>   'unknownArtist',
>   'unknownSong').then((unknownTitle, unknownCount, unknownArtist,
> unknownSong) => {
>     db.title = unknownTitle;
>     db.count = unknownCount;
>     db.artist = unknownArtist;
>     db.song = unknownSong;
> });

I'm not sure which one is more common right now, but I'm almost certain that the above one is the one that we should encourage.  It makes it clear which strings are being used where.

> // 2:
> 
> const l10nKeys = [
>   'unknownTitle',
>   ['unknownCount', {'count': db.unknownCount}],
>   'unknownArtist',
>   'unknownSong'
> ];
> 
> document.l10n.formatValues(...l10nKeys).then((translations) => {
>   db.title = translations[0];
>   db.count = translations[1];
>   db.artist = translations[2];
>   db.song = translations[3];
> });
> 
> And the latter is a more common case from my experience. That's why I vote
> for an Array.

Wouldn't you agree that the latter is a worse API overall? Indexes are cryptic and don't mean anything without looking at the original l10nKeys array.  Destructuring as in the first snippet is much more comprehensive and maintainable.

And if you really want to use indexes (which I think you shouldn't!), you can do it with a Map as well:

  document.l10n.formatValues(...l10nKeys).then(translations => {
    db.title = translations.get(l10nKeys[0]);
    db.count = translations.get(l10nKeys[1]);
    db.artist = translations.get(l10nKeys[2]);
    db.song = translations.get(l10nKeys[3]);
  });

The Map gives you the same possibilities as the Array, plus more, because you can also do map.get('id').  What's the advanatge of the Array?

Comment 23

[Staś Małolepszy :stas]

Ah, mid-aired with your comment.

Comment 24

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

Wait, you mean I can destruct a map?

Comment 25

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> Wait, you mean I can destruct a map?

Yep :)  See my comment 14:

>   document.l10n.formatValues(
>     'foo1', ['foo2', {name: 'John'}], 'foo3'
>   ).then(
>     ([foo1, foo2, foo3]) => console.log(foo2[1]));

If you destruct the whole map, foo2 is [ ['foo2', {name: 'John'}], 'something' ].

> or:
> 
>   document.l10n.formatValues(
>     'foo1', ['foo2', {name: 'John'}], 'foo3'
>   ).then(
>     translationMap => {
>       const [foo1, foo2, foo3] = translationMap.values();
>       console.log(foo2);
>     });

Here I'm only destructuring values.

Comment 26

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

I don't think it works the way you are suggesting.

Taking this example:

// Map Approach

var func = ([unknownTitle, unknownCount, unknownArtist, unknownSong]) => {
  console.log('Title: ' + unknownTitle);
  console.log('Count: ' + unknownCount);
  console.log('Artist: ' + unknownArtist);
  console.log('Song: ' + unknownSong);
}

var map = new Map();
map.set('unknownTitle', 'Unknown Title');
map.set(['unknownCount', {count: 2}], 'Unknown Count: 2');
map.set('unknownArtist', 'Unknown Artist');
map.set('unknownSong', 'Unknown Song');

func(map);

// Array approach

var array = [
  'Unknown Title',
  'Unknown Count: 2',
  'Unknown Artist',
  'Unknown Song'
];

func(array);


Array returns exactly what I believe the API users would expect. Map returns something that requires twisting.

Comment 27

[Staś Małolepszy :stas]

Destructuring a map gives you tuples of (key, value).  If you only need values, you need to destructure map.values().  Can we assume that people writing JavaScript know how to write JavaScript? :)

Comment 28

[Staś Małolepszy :stas]

Sorry, accidentally submitted the comment too soon.

(In reply to Staś Małolepszy :stas from comment #27)
> Destructuring a map gives you tuples of (key, value).  If you only need
> values, you need to destructure map.values().  Can we assume that people
> writing JavaScript know how to write JavaScript? :)

If we say that the formatValues method resolves to a Map of keys+translations, I think it's enough for people to understand how to use it.  And they can do whatever they wish with it.  My only point is that I feel like a Map gives us the same things as Array plus more.  I'm not sure if we agree on this or not?

Comment 29

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

> If we say that the formatValues method resolves to a Map of keys+translations, I think it's enough for people to understand how to use it.

I'm not concerned if people will figure out how to use it, but over if we provide them optimal API for their use, or we're providing them artificial API that they have to work around.

>  My only point is that I feel like a Map gives us the same things as Array plus more.  I'm not sure if we agree on this or not?

I think that the difference is that you seem to focus on what gives "more", while I am trying to decide on what return value is going to provide the cleanest, most readable code.

I'm just not sure if Map is really a better API here and the value of iterating over keys vs. indexes seems much less of a benefit to me than the fact that destructing will feel awkward (the returned values won't be values).

Comment 30

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #29)

> I think that the difference is that you seem to focus on what gives "more",
> while I am trying to decide on what return value is going to provide the
> cleanest, most readable code.

I'm here for the same reason :)

Let's try to summarize and reach conclusion:

Arrays can be used via:

 - destructuring,
 - iteration,
 - indexes

Maps can be used via:

 - destructuring of the whole map,
 - destructuring of values,
 - iteration,
 - keys.

I think indexes are a terrible way of accessing values.  As long as we never show code to anyone which uses indexes, arrays and maps are the same, but maps give you get().  If you don't care about get(), go for an array.  Just don't use indexes.  Use destructuring or iteration.  Please.

Comment 31

[Staś Małolepszy :stas]

Pasting my braindump from IRC for completeness:

20:56 <@stas> I like maps because I'd like to forbid using indexes
20:56 <@stas> there's a part of the API design which is aspirational:  this is what we want devs to be doing
20:57 <@stas> I want them to use destructruing or iteration
20:57 <@stas> definitely not indexes, because they make the code less readable
20:57 <@stas> if we can agree on that, I don't care if it's a map or an array
20:57 <@stas> map is a vehicle for my sentiment that indexes are bad
20:58 <@stas> it's a bit like with l10n.ready
20:59 <@stas> we could explain to people that sometimes they need to wrap formatValue with l10n.ready, and sometimes not
20:59 <@stas> the end result is that people wrap everything, just in case
20:59 <@stas> so we embrace this and build a new formatValue which doesn't require l10n.ready, ever
21:00 <@stas> here, I think devs will start writing bad code using indexes
21:00 <@stas> even if they don't have ti
21:00 <@stas> to*
21:00 <@stas> is there a way to encourage them to use destructuring or iteration?
21:00 <@stas> in my mind, Map was this way
21:01 <@stas> obviously, programmatically, Array works too
21:02 <@stas> if you think indexes aren't big of a deal or that we can encourage people to do the right thing, let's use an array.

Comment 32

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

Comment on attachment 8660093 [details] [review]
pull request

Updated the patch and added tests. Rerequesting review.

Comment 33

[Staś Małolepszy :stas]

Comment on attachment 8660093 [details] [review]
pull request

Sweet, r=me.  Great job!

Comment 34

[Staś Małolepszy :stas]

This has landed in the l20n.js repo in https://github.com/l20n/l20n.js/commit/5443b9acca685f4545b92f0844b771be406672af.  Marking fixed.