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

RESOLVED FIXED in 1.0 beta

Status

P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gandalf, Assigned: stas)

Tracking

unspecified
1.0 beta
x86
Mac OS X
Bug Flags:
blocking-parkcity +

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
This will enable us to trigger node retranslation on changes.

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

Updated

6 years ago
Assignee: nobody → stas
Priority: -- → P2
Target Milestone: --- → 1.0 beta
(Assignee)

Updated

6 years ago
Blocks: 847012
(Assignee)

Comment 1

6 years ago
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.
Attachment #725133 - Flags: feedback?(gandalf)
(Reporter)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
(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?
(Reporter)

Comment 4

6 years ago
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
(Assignee)

Comment 5

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

Comment 6

6 years ago
good point, let's stick to that for now then
(Assignee)

Updated

6 years ago
Flags: blocking-parkcity+
(Assignee)

Comment 7

6 years ago
With bug 850855 fixed, I can now land this, as the function signatures are now compatible.
(Assignee)

Comment 8

6 years ago
https://github.com/l20n/l20n.js/commit/c23640e2e10716ae1331da0be463b427bf3aa510
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Attachment #725133 - Flags: feedback?(gandalf)
You need to log in before you can comment on or make changes to this bug.