847013 - Compiler should provide meta information on variables and globals used to compute the entity

Status: RESOLVED FIXED

Description

[Zibi Braniecki [:gandalf][:zibi]]

This will enable us to trigger node retranslation on changes.

See bug 847012 for the work on context/globals/html bindings side.

Comment 1

[Staś Małolepszy :stas]

Created attachment 725133 [details] [diff] [review]
Patch proposal

G, something like this?

I tested this quite extensively with some async scenarios using setTimeout and process.nextTick in node, and it didn't break.

Our gets are around 2% slower compared to the code which didn't collect globals references.

Comment 2

[Zibi Braniecki [:gandalf][:zibi]]

Comment on attachment 725133 [details] [diff] [review]
Patch proposal

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

::: lib/compiler.js
@@ +293,5 @@
> +      _references.globals = {};
> +      return {
> +        value: this.toString(ctxdata),
> +        globals: _references.globals,
> +      }

document pls that this.toString rebuilds globals

@@ +455,4 @@
>            throw new RuntimeError('Reference to an unknown global: ' + name,
>                                   entry);
>          }
> +        _references.globals[name] = true;

if that's all we store, would it make sense to keep globals as a list instead?

Comment 3

[Staś Małolepszy :stas]

(In reply to Zbigniew Braniecki [:gandalf] from comment #2)
> Comment on attachment 725133 [details] [diff] [review]
> Patch proposal
> 
> Review of attachment 725133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/compiler.js
> @@ +293,5 @@
> > +      _references.globals = {};
> > +      return {
> > +        value: this.toString(ctxdata),
> > +        globals: _references.globals,
> > +      }
> 
> document pls that this.toString rebuilds globals

Right.

> 
> @@ +455,4 @@
> >            throw new RuntimeError('Reference to an unknown global: ' + name,
> >                                   entry);
> >          }
> > +        _references.globals[name] = true;
> 
> if that's all we store, would it make sense to keep globals as a list
> instead?

I thought about this but then we'd need to de-duplicate the list (or have an implementation of a Python-like set?) so that we don't set too many listeners.  Did I get that right?

Comment 4

[Zibi Braniecki [:gandalf][:zibi]]

Umm, can we use ES6 Set? :)
http://www.nczonline.net/blog/2012/09/25/ecmascript-6-collections-part-1-sets/

Fx and Chrome support it

Comment 5

[Staś Małolepszy :stas]

I'd love to use more ES6, or even some extensions to ES5, but for compatibility with node, we should stick to pure ES5.

Comment 6

[Zibi Braniecki [:gandalf][:zibi]]

good point, let's stick to that for now then

Comment 7

[Staś Małolepszy :stas]

With bug 850855 fixed, I can now land this, as the function signatures are now compatible.

Comment 8

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/c23640e2e10716ae1331da0be463b427bf3aa510