847012 - Rewrite globals support to be class based and be able to trigger retranslation on changes

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P2)

Description

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

This will enable node retranslation on global's value change.

Comment 1

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

Stas: can you give me feedback on this code?
https://github.com/zbraniecki/l20n.js/compare/master...responsive

Comment 2

[Staś Małolepszy :stas]

Adding APIchange in case we need ctx.responsive = [true|false] to toggle this behavior.

Comment 3

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #1)
> Stas: can you give me feedback on this code?
> https://github.com/zbraniecki/l20n.js/compare/master...responsive

I'm sure I typed this somewhere before (email?), but it's not in this bug, so here goes my feedback:

+Global.prototype.hook = function(globals, get) {
+  var group = globals;
+  if (this.group) {
+    if (!globals[this.group]) {
+      globals[this.group] = {};
+    }
+    group = globals[this.group];
+  }
+  Object.defineProperty(group, this.id, {get: get,
+    enumerable: true,
+    configurable: true});
+} 

I'd remove the 'group' approach entirely and replace it with @screenWidth (do we need @screenHeight?) for 1.0, and, possibly, @screen in 2.0.  @screen would be an object with 'width' and 'height' as members (maybe add 'dpi'?)

This will allow you to remove the clunky defineProperty, too.


+L20n._globals.push(ScreenWidthGlobal);
+L20n._globals.push(OSGlobal);
+L20n._globals.push(HourGlobal); 

I'm not a fan putting _globals directly on L20n.  I wonder if there's a better way.


+    for (var i in L20n._globals) {
+      var global = new L20n._globals[i];
+      global.init(_globals);
+      _globalsUsage[global.id] = [];
+      global.addEventListener('change', function(id) { 

There's too much of the word 'global' in this code, which makes it hard to read.  Maybe use more descriptive names, like _globalsContructor?

Comment 4

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #3)
> I'm sure I typed this somewhere before (email?), but it's not in this bug,
> so here goes my feedback:

I implemented some of this on my branch:

https://github.com/stasm/l20n.js/commit/9600357408a964bc7baaa7b5e75e84e125c8303c

> +L20n._globals.push(ScreenWidthGlobal);
> +L20n._globals.push(OSGlobal);
> +L20n._globals.push(HourGlobal); 
> 
> I'm not a fan putting _globals directly on L20n.  I wonder if there's a
> better way.

I didn't find anything significantly better, but I did rename L20n._globals to _globalContructors.

I wasn't able to make the callback work properly.  I think there's some logic missing that re-registers the callback once they are fired once.  I don't have time to look into it right now, but hopefully you'll be able to figure it out and land this.

Take a look at the change in the compiler, too.

Comment 5

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #4)

> I wasn't able to make the callback work properly.  I think there's some
> logic missing that re-registers the callback once they are fired once.  I
> don't have time to look into it right now, but hopefully you'll be able to
> figure it out and land this.

Found it:

https://github.com/stasm/l20n.js/commit/e5dff4e49ace7504b87a8abaf2259505a101ff9e

Comment 6

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #4)

> I didn't find anything significantly better, but I did rename L20n._globals
> to _globalContructors.

How about this?

https://github.com/stasm/l20n.js/commit/167c7702af6da15e04f477e8328ce60e9bacee9c

Comment 7

[Staś Małolepszy :stas]

Attachment: Patch based on Gandalf's 'responsive' branch

Gandalf, I added a few more things and I think this is close to what we'll want to land.  I'll let you review and decide.

You can see the history of my changes at https://github.com/stasm/l20n.js/commits/responsive, too.

Comment 8

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

Comment on attachment 739255 [details] [diff] [review]
Patch based on Gandalf's 'responsive' branch

it's extremely far from what we want or can land.

I'm working based on your patches, but there's a high pile of unwanted memleaks coming into play here.

Comment 9

[Staś Małolepszy :stas]

Comment on attachment 739255 [details] [diff] [review]
Patch based on Gandalf's 'responsive' branch

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

::: lib/globals.js
@@ +6,5 @@
> + * We want to set the listener on resize and timeout on hour only if
> + * there's some use case so after first get() probably.
> + *
> + * Question is - do we want to track use of the global and stop the listeners
> + * if we stop using the global? How to track that?

I think this taken care of automatically in context's get*.  When the value of an entity stops using a global, the callback will not be registered.

@@ +24,5 @@
> +
> +Global.prototype.destroy = function() {
> +}
> +
> +this.Global = Global;

Oops, this pollutes the global namespace.  New patch coming in a second.

@@ +103,5 @@
> +HourGlobal.prototype.constructor = HourGlobal;
> +
> +L20n.registerGlobal(ScreenWidthGlobal);
> +L20n.registerGlobal(OSGlobal);
> +L20n.registerGlobal(HourGlobal);

I'll leave this here for now, but I wonder what we should do for the server-side case.

Comment 10

[Staś Małolepszy :stas]

Attachment: Patch 2

Patch v.2, with fixes mentioned in comment 9.

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 739262 [details] [diff] [review]
Patch 2

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

::: lib/context.js
@@ +292,5 @@
> +          vals[iot] = getEntity(iot);
> +          for (var global in vals[iot].globals) {
> +            _globals.listeners[global].push(function() {
> +              callback(getMany(idsOrTuples, callback));
> +            });

This code looks wrong.  First of all, we should register the listeners also in the iot-is-an-array case.

More importantly though, I believe we should only fire the callback once, regardless of how many entities actually used the global in question.

Comment 12

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

Comment on attachment 739262 [details] [diff] [review]
Patch 2

stas, the whole approach has to be adjusted to stop the memleaks. We can't collect callbacks per global.

We have to store each callback and its globals separately to avoid cases when one global retranslates and another is pending on the same callback.

Comment 13

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

Attachment: patch v3

Comment 14

[Staś Małolepszy :stas]

Comment on attachment 740709 [details] [diff] [review]
patch v3

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

Nice work!  r=me with comments below.

::: lib/context.js
@@ +251,2 @@
>        }
> +      return getMany(idsOrTuples, callback);

Why remove the event listener here?

@@ +265,5 @@
> +      Object.keys(vals).forEach(function (key) {
> +        Object.keys(vals[key].globals).forEach(function (key2) {
> +          globalsUsed[key2] = true;
> +        });
> +      });

You're already iterating over idsOrTuples once, so you can avoid doing it a second time in "Object.keys(vals).forEach" like so:

     var globalsUsed = {};
      for (var i = 0, iot; iot = idsOrTuples[i]; i++) {
        var id;
        if (Array.isArray(iot)) {
          id = iot[0];
          vals[id] = getEntity(id, iot[1]);
        } else {
          id = iot;
          vals[id] = getEntity(id);
        }
        for (var global in vals[id].globals) {
          if (vals[id].globals.hasOwnProperty(global)) {
            globalsUsed[global] = true;
          }
        }
      }

::: lib/globals.js
@@ +5,5 @@
> + * We want to set the listener on resize and timeout on hour only if
> + * there's some use case so after first get() probably.
> + *
> + * Question is - do we want to track use of the global and stop the listeners
> + * if we stop using the global? How to track that?

Do we still need this comment now?

@@ +9,5 @@
> + * if we stop using the global? How to track that?
> + *
> + **/
> +
> +var GlobalsManager = function() {

I'd prefer:

function GloablsManager() {}

(same with globals constructors)

@@ +35,5 @@
> +      }
> +    });
> +  };
> +
> +  function registerGet(get) {

I'm not a huge fan of "register" here.  Maybe: addGlobalListener(get)?

@@ +53,5 @@
> +        });
> +      }
> +    } else {
> +      if (get.globals.length == 0) {
> +        delete(_usage[usageInc]);

Have you tried Array.splice?

Or, could _usage be a dictionary-like object?  Then, instead of

  _usage.push(get);

you could do:

  _usage[get] = true;

Same with inUsage.  This would prevent the _usage array from becoming sparse. You could also use "in" in lieu of "indexOf".

@@ +109,5 @@
> + * We want to have @screen.width, but since we can't
> + * get it from compiler, we call it @screen and in order
> + * to keep API forward-compatible with 1.0 we return 
> + * an object with key width to make it callable as @screen.width
> + */

Nit:  can you format this with textwidth=79, change the comment character to //, put a TODO or XXX and file a bug about this and mention it here?

@@ +110,5 @@
> + * get it from compiler, we call it @screen and in order
> + * to keep API forward-compatible with 1.0 we return 
> + * an object with key width to make it callable as @screen.width
> + */
> +var ScreenWidthGlobal = function() {

As per comment above, call this ScreenGlobal for now?

@@ +224,5 @@
> +HourGlobal.prototype.constructor = HourGlobal;
> +
> +GlobalsManager.registerGlobal(ScreenWidthGlobal);
> +GlobalsManager.registerGlobal(OSGlobal);
> +GlobalsManager.registerGlobal(HourGlobal);

\o/

Glad we don't have to use the L20n object here.

Comment 15

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

(In reply to Staś Małolepszy :stas (needinfo along with cc, please; if you want an r+ from me, attach your patch to the bug; I don't review pull requests) from comment #14)

> @@ +35,5 @@
> > +      }
> > +    });
> > +  };
> > +
> > +  function registerGet(get) {
> 
> I'm not a huge fan of "register" here.  Maybe: addGlobalListener(get)?

That would send the wrong message. We're not adding listeners here. We're registering for gets. Quite often we won't even add a new entry here...

If registerGet is against your feng shui, maybe manageGlobalsListener?

> @@ +53,5 @@
> > +        });
> > +      }
> > +    } else {
> > +      if (get.globals.length == 0) {
> > +        delete(_usage[usageInc]);
> 
> Have you tried Array.splice?

Yeah, much slower.
http://jsperf.com/object-delete-vs-array-splice-vs-array-delete/6

> Or, could _usage be a dictionary-like object?  Then, instead of
> 
>   _usage.push(get);
> 
> you could do:
> 
>   _usage[get] = true;

Nope, object keys are stringified.

> Same with inUsage.  This would prevent the _usage array from becoming
> sparse. You could also use "in" in lieu of "indexOf".

inUsage is null or a reference to an array element. How would it benefit from the proposed change?

Comment 16

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #15)
> That would send the wrong message. We're not adding listeners here. We're
> registering for gets. Quite often we won't even add a new entry here...
> 
> If registerGet is against your feng shui, maybe manageGlobalsListener?

Nah.. How about "bind", my new favorite word? :)  _globalsManager.bind(get)

> Nope, object keys are stringified.

TIL.

> inUsage is null or a reference to an array element. How would it benefit
> from the proposed change?

It would, I meant inUsage.globals and get.globals :)

Comment 17

[Staś Małolepszy :stas]

I landed the patch on Gandalf's behalf:

https://github.com/l20n/l20n.js/commit/857de22299cfcd20ba5eba29b5ad0e2a0ef77f03