Closed
Bug 847013
Opened 12 years ago
Closed 12 years ago
Compiler should provide meta information on variables and globals used to compute the entity
Categories
(L20n :: JS Library, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
1.0 beta
People
(Reporter: zbraniecki, Assigned: stas)
References
Details
Attachments
(1 file)
2.28 KB,
patch
|
Details | Diff | Splinter Review |
This will enable us to trigger node retranslation on changes.
See bug 847012 for the work on context/globals/html bindings side.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → stas
Priority: -- → P2
Target Milestone: --- → 1.0 beta
Assignee | ||
Comment 1•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
||
good point, let's stick to that for now then
Assignee | ||
Updated•12 years ago
|
Flags: blocking-parkcity+
Assignee | ||
Comment 7•12 years ago
|
||
With bug 850855 fixed, I can now land this, as the function signatures are now compatible.
Assignee | ||
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Attachment #725133 -
Flags: feedback?(gandalf)
You need to log in
before you can comment on or make changes to this bug.
Description
•