850855 - Clean up context's get*

Status: RESOLVED FIXED

(L20n :: JS Library, defect, P1)

Description

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

We currently have a mess inside get* functions. Both code wise and API wise.

We need to clean this up. Arguments, return values, code flow, fallbacks, error reporting.

Comment 1

[Staś Małolepszy :stas]

Attachment: Patch

The patch implements a recursive getFromLocale instead of a _get with a while loop.

It also defaults to getEntity.  When doing ctx.get, the code uses getEntity but just returns the string value.

There shouldn't be any perf implications:  most of the times, if you care about the attrs, you'll use ctx.getEntity anyways.  If you're using ctx.get, it's probably with an entity which doesn't have any public attributes.

Comment 2

[Staś Małolepszy :stas]

Attachment: Patch (rebased)

I rebased the patch on top of the current master.

Comment 3

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

Comment on attachment 737562 [details] [diff] [review]
Patch (rebased)

>diff --git a/lib/compiler.js b/lib/compiler.js
>index 85e55e7..5c889a6 100644
>--- a/lib/compiler.js
>+++ b/lib/compiler.js
>@@ -235,6 +235,7 @@
>       this.local = node.local || false;
>       this.index = [];
>       this.attributes = {};
>+      this.publicAttributes = [];
>       var i;
>       for (i = 0; i < node.index.length; i++) {
>         this.index.push(Expression(node.index[i], this));
>@@ -242,6 +243,9 @@
>       for (i = 0; i < node.attrs.length; i++) {
>         var attr = node.attrs[i];
>         this.attributes[attr.key.name] = new Attribute(attr, this);
>+        if (!attr.local) {
>+          this.publicAttributes.push(attr.key.name);
>+        }
>       }
>       this.value = Expression(node.value, this, this.index);
>     }
>@@ -284,8 +288,8 @@
>         value: this.getString(ctxdata),
>         attributes: {}
>       };
>-      for (var name in this.attributes) {
>-        entity.attributes[name] = this.attributes[name].getString(ctxdata);
>+      for (var i = 0, attr; attr = this.publicAttributes[i]; i++) {
>+        entity.attributes[attr] = this.attributes[attr].getString(ctxdata);
>       }

Why is publicAttributes better than checking for .local here?

>diff --git a/lib/context.js b/lib/context.js
>index 23d40e7..376d625 100644
>--- a/lib/context.js
>+++ b/lib/context.js
>@@ -204,13 +204,46 @@
>       },
>     };
>     var _listeners = [];
>-    var _pending = [];
>+    var _pending = {
>+      get: [],
>+      getEntity: [],
>+    };

Can we not use _pending here and require context to be ready before any get is fired?

I think it would be suitable for 1.0, extendable for Next and would simplify the code.

Comment 4

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #3)
> Comment on attachment 737562 [details] [diff] [review]
> Patch (rebased)
> 
> >diff --git a/lib/compiler.js b/lib/compiler.js
> >index 85e55e7..5c889a6 100644
> >--- a/lib/compiler.js
> >+++ b/lib/compiler.js
> >@@ -235,6 +235,7 @@
> >       this.local = node.local || false;
> >       this.index = [];
> >       this.attributes = {};
> >+      this.publicAttributes = [];
> >       var i;
> >       for (i = 0; i < node.index.length; i++) {
> >         this.index.push(Expression(node.index[i], this));
> >@@ -242,6 +243,9 @@
> >       for (i = 0; i < node.attrs.length; i++) {
> >         var attr = node.attrs[i];
> >         this.attributes[attr.key.name] = new Attribute(attr, this);
> >+        if (!attr.local) {
> >+          this.publicAttributes.push(attr.key.name);
> >+        }
> >       }
> >       this.value = Expression(node.value, this, this.index);
> >     }
> >@@ -284,8 +288,8 @@
> >         value: this.getString(ctxdata),
> >         attributes: {}
> >       };
> >-      for (var name in this.attributes) {
> >-        entity.attributes[name] = this.attributes[name].getString(ctxdata);
> >+      for (var i = 0, attr; attr = this.publicAttributes[i]; i++) {
> >+        entity.attributes[attr] = this.attributes[attr].getString(ctxdata);
> >       }
> 
> Why is publicAttributes better than checking for .local here?

publicAttributes is filled in during the initialization of the Entity, so only once.  Checking for .local here means checking every time we call Entity.getEntity (I'll rename this to Entity.get).

From my tests, on an attribute-sparse file, there's negligible performance difference.  For attribute-heavy files, publicAttributes approach can be up to 15% faster,


> >diff --git a/lib/context.js b/lib/context.js
> >index 23d40e7..376d625 100644
> >--- a/lib/context.js
> >+++ b/lib/context.js
> >@@ -204,13 +204,46 @@
> >       },
> >     };
> >     var _listeners = [];
> >-    var _pending = [];
> >+    var _pending = {
> >+      get: [],
> >+      getEntity: [],
> >+    };
> 
> Can we not use _pending here and require context to be ready before any get
> is fired?
> 
> I think it would be suitable for 1.0, extendable for Next and would simplify
> the code.

OK, I'll change it.

Comment 5

[Staś Małolepszy :stas]

Attachment: Patch (addresses review comments)

This patch also fixes bug 862101.

Comment 6

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/9e62490ff39a6761c7a85117fe4721796d75d53e

Comment 7

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

Is it possible that this regressed string compilation? It seems like the strings are always returned as simple ones now.

Comment 8

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

false alarm, sorry.