1191011 - Provide a cache helper for Intl Formatters

Status: RESOLVED FIXED

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

Description

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

Intl DateTime and Number formatters are extremely useful because they are much faster than formatting Date and Number on each call.

At the same time they also need to be cached themselves, so that they are not created in each call.

And lastly, they have to be recreated after timeformatchange/languagechange.

A helper with that would help apps do this properly and also unify how they present date, time and numbers.

Comment 1

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

Attachment: intl_helper.js

POC. What do you think guys?

The way people would use it is by injecting the helper and then:

IntlHelper.defineFormatter('datetime', 'time', {
  hour: 'numeric',
  minute: 'numeric'
});
IntlHelper.defineFormatter('datetime', 'full', {
  month: '2-digit',
  day: '2-digit',
  year: 'numeric'
});

and then call it:

var formatter = IntlHelper.getFormatter('datetime', 'full');
element.textContent = formatter.format(d);

and could listen to events:

IntlHelper.addEventListener('datetimechange', updateDateTimeElements);
IntlHelper.addEventListener('numberchange', updateNumberElements);

In the future we could extend it to currencies, collations for sorting etc.

Comment 2

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

See bug 1187878 for an example where we could use it already

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 8643223 [details]
intl_helper.js

I don't have a strong opinion.

Comment 4

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8643223 [details]
intl_helper.js

I like the idea.

Below some comments.


>(function(){
>  'use strict';
>
>  var formatters;
>  var listeners;

We'll need a way to reset both listeners and formatters for unit tests.

>
>  var IntlHelper = {
>    defineFormatter: function(type, name, options) {
>      if (!formatters) {
>        formatters = Object.create(null);
>      }
>      if (!(type in formatters)) {
>        formatters[type] = Object.create(null);
>      }
>      formatters[type][name] = { options, object: undefined };
>    },
>    getFormatter: function(type, name) {
>      const formatter = formatters[type][name];
>      if (formatter.object === undefined) {
>        switch (type) {
>          case 'number':
>            formatter.object = Intl.NumberFormatter(
>              navigator.languages,
>              formatter.options
>            );
>            break;
>          case 'datetime':
>            const options = Object.assign({}, formatter.options);
>            if (options.hour) {
>              options.hour12 = navigator.mozHour12;
>            }
>            formatter.object = Intl.DateTimeFormatter(
>              navigator.languages,
>              options
>            );
>            break;
>          default:
>            throw new Error('Unknown type');

I think we should `throw` in defineFormatter as well.
You could use a constant + a function `ensureValidType` in the global scope (kind of like we do in [1] except we'd have a static list here).

[1] https://github.com/mozilla-b2g/gaia/blob/2d7f369fd923b6df3b00d76844c413c1202c04ba/shared/js/event_dispatcher.js#L77-L81

>        }
>      }
>      return formatter.object;
>    },

I like it that we populate the cache in `getFormatter` and not in `defineFormatter`.


>    addEventListener: function(type, cb) {
>      if (!listeners) {
>        listeners[type] = [];
>      }
>      listeners[type].push(cb);
>    },
>    removeEventListener: function(type, cb) {
>      if (!(type in listeners)) {
>        return;
>      }
>      const pos = listeners[type].indexOf(cb);
>
>      if (pos === -1) {
>        return;
>      }
>
>      listeners[type].splice(pos, 1);
>    },

I'm not a huge fan of using `addEventListener` and `removeEventListener`, because users would have expectations that they work as their DOM counterparts: esp dupe checking, can register an object with handleEvent.
So I think `on`/`off` would be a better fit here. At least it makes it clear it's not a DOM-like implementation.


>    emit: function(type, ...args) {
>      if (!(type in listeners)) {
>        return;
>      }
>      listeners[type].slice().forEach(listener => listener.apply(this, args));
>    }
>  };
>
>  function resetDateTime(reason) {
>    // XXX: we could be smarter here and reset on timeformatchange only
>    // if the formatter uses 'hour' option
>
>    // XXX: we may also want to figure out how to avoid sending the same event
>    // twice in case languagechange triggers timeformatchange.
>    // One way to do this would be to use 'reason' in timeformatchange
>    if (formatters && 'datetime' in formatters) {
>      for (let formatter of formatters['datetime']) {
>        formatter.object = undefined;
>      }
>    }
>    IntlHelper.emit('datetimechange', {reason: reason});
>  }
>
>  function resetNumber(reason) {
>    if (formatters && 'number' in formatters) {
>      for (let formatter of formatters['number']) {
>        formatter.object = undefined;
>      }
>    }
>    IntlHelper.emit('numberchange', {reason: reason});
>  }

I'd emit the event in the event handler below; so that the resetXXX functions only do what their names suggest.
Moreover this makes it possible in the `languagechange` event to emit both events only after we did reset both types.

>
>  window.addEventListener('timeformatchange', () => {
>    resetDateTime('timeformatchange');
>  });
>
>  navigator.addEventListener('languagechange', () => {
>    resetDateTime('languagechange');
>    resetNumber('languagechange');
>  });
>})();

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8643223 [details]
intl_helper.js

I agree with Julien's comments.  A few other thoughts follow below.  Overall, f=me,thanks!


>  var IntlHelper = {
>    defineFormatter: function(type, name, options) {

Switch the order of `name` and `type`?


>      if (!formatters) {
>        formatters = Object.create(null);
>      }
>      if (!(type in formatters)) {
>        formatters[type] = Object.create(null);
>      }
>      formatters[type][name] = { options, object: undefined };

Do we need this nested hierarchy here?  If we require the `name` to be unique then we could get away with a flatter cache structure and use .filter() on values in reset* methods below.

Also, perhaps we could use a Map()?


>    },
>    getFormatter: function(type, name) {
>      const formatter = formatters[type][name];

Again, if names are unique, getFormatter could have a simpler signature: getFormatter(name).  Type would be stored in the cache:

  formatters[name] = { options, object: undefined, type: type };


>    addEventListener: function(type, cb) {
>      if (!listeners) {
>        listeners[type] = [];

This looks wrong; `listeners` is still undefined here. Can we initialize it to an empty Map, with a Set for each type?  This will make it easier to remove callbacks.


>    emit: function(type, ...args) {

Is `emit` part of the public API?


>  function resetDateTime(reason) {
>    IntlHelper.emit('datetimechange', {reason: reason});

In order to better comply with the name of the function we're in, maybe move IntlHelper.emit() to window and navigator listeners below?


>  window.addEventListener('timeformatchange', () => {
>    resetDateTime('timeformatchange');
>  });
>
>  navigator.addEventListener('languagechange', () => {
>    resetDateTime('languagechange');
>    resetNumber('languagechange');
>  });

Perhaps this could be replace with a single handleEvent?

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

Comment 7

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

Comment on attachment 8660239 [details] [review]
[gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

Julien, Stas, new round of feedback?

The internal code could use some cleanups and can be compressed but before I do this, I'd like to know if you like the API.

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8660239 [details] [review]
[gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

I like this a lot!  I'm just not sure about the approach you've taken re. emitting and listening to changes.  I think I might prefer the old scheme where you register listeners for (type + 'change') events.  But I'm not strongly attached to doing it either way.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Comment on attachment 8660239 [details] [review]
[gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

Left some comments on github; I don't have a really strong opinion about all this though.

Comment 10

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

Comment on attachment 8660239 [details] [review]
[gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

First try.

I think code-wise the patch is complete. I added twice as much test code as helper code. I think it's ready for the review.

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8660239 [details] [review]
[gaia] zbraniecki:1191011-intl-helper > mozilla-b2g:master

Yay, that's a lot of tests :)

r=me with two major suggestions:

 - let's guard against buggy callbacks by wrapping them in try...catch.  Or, let's be explicit in that it's the devs responsibility to ensure the callback doesn't throw by adding it to the docs and to the tests,

 - I suggest we remove affectedTypes which add another layer of abstraction (affected types -> affected objects -> affected callback) and simply use isAffected to filter callbacks to run.

Nice work overall!

Comment 12

[Staś Małolepszy :stas]

Once we have Intl-formatting macros in L20n, will you want to move this helper into L20n?

Comment 13

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

(In reply to Staś Małolepszy :stas from comment #12)
> Once we have Intl-formatting macros in L20n, will you want to move this
> helper into L20n?

I don't know. I like that people can use it independently of l20n.js (I can imagine an app that doesn't use l20n.js but does use IntlHelper), but we may also need something similar inside our macro code.

Comment 14

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

Thanks a lot Stas! I applied *all* your suggestions. Great review :)

Commit: https://github.com/mozilla-b2g/gaia/commit/50fa6d1dc7532ece58ad689ccfabe10c14797b98