Improve JS export mechanism

... to enable property renaming, in JS runtime compilation.

[johnfargo, 2011-02-23 01:07:00]

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java
(right):

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:65:
static final String EXPORT_JS_FUNCTION =
let's put all export code into a feature ("exportjs") of its own, so we can
write JsUnit tests for it, include it in dependency resolution, etc.

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:66:
"function exportGlobalJs(props) {" +
by my read, exportLocalJs can do everything exportGlobalJs can -- just without a
"props" argument, ie:

exportLocalJs("gadgets", [ gadgets ]);

export-generation code should be responsible for generating these. It emits a
"props" argument only when there was a top-level ns/object.

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:91:
"  } else if (typeof base === 'function') {" +
As noted in a separate thread, we need to augment this function to support
prototype-style objects.

The function should stop at token "prototype", then replace the prefix to that
point with wrapper function like:
var exportedFn = function() {
  this.constructor = base.constructor;
  this.__proto__ = base.__proto__; // maybe
  exportProps(this);
};

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:175:
result.append("exportLocalJs('").append(input.namespace).append("',[");
nit: public static final String for the function name

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:195:
private String getNamespace(String symbol) {
as noted above, you could just return the full string if !contains("."), and
return no properties in that case.

[mhermanto, 2011-02-23 19:24:59]

On Tue, Feb 22, 2011 at 5:07 PM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java
> (right):
>
>
>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:65:
> static final String EXPORT_JS_FUNCTION =
> let's put all export code into a feature ("exportjs") of its own, so we
> can write JsUnit tests for it, include it in dependency resolution, etc.
>

Agree on testability, but where to put the dependency resolution? We can do
it in JsHandler (if >=1 exports statements, add feature=exportjs to needed
bundles), but I'm hoping that resolution is better encapsulated in 1 place,
ie: the compiler should just chew raw JS string (not deal with feature
augmentation).

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:66:
> "function exportGlobalJs(props) {" +
> by my read, exportLocalJs can do everything exportGlobalJs can -- just
> without a "props" argument, ie:
>
> exportLocalJs("gadgets", [ gadgets ]);
>
> export-generation code should be responsible for generating these. It
> emits a "props" argument only when there was a top-level ns/object.
>

Right. Removed exportGlobalJs.


>
>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:91:
> "  } else if (typeof base === 'function') {" +
> As noted in a separate thread, we need to augment this function to
> support prototype-style objects.

The function should stop at token "prototype", then replace the prefix
> to that point with wrapper function like:
> var exportedFn = function() {
>  this.constructor = base.constructor;
>  this.__proto__ = base.__proto__; // maybe
>  exportProps(this);
> };
>
> But, it seems to work fine as-is --

// given ...
gadgets.lib4 = function() {};
gadgets.lib4.prototype.unquoted = function() { return "foo" };
// gets compiled to ...
a.b.prototype.a = function $f() { return"foo" };
d("gadgets.lib4.prototype", [a, a.b, a.b.prototype], {a:"unquoted"}); //
exportLocalJs
// which results ...
gadgets.lib4.prototype.a = $f
gadgets.lib4.prototype.unquoted = $f

This is what we expect, no?  The logic sounds sane to me, given the input
"gadgets.lib4.prototype", after traversal, base becomes the prototype, which
is of type 'object' (not type 'function'), which then exportProps() like
normal (on the prototype). So, I don't think we need to stop at token
'prototype'.

http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:175:
> result.append("exportLocalJs('").append(input.namespace).append("',[");
> nit: public static final String for the function name
>

Done.


>
>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:195:
> private String getNamespace(String symbol) {
> as noted above, you could just return the full string if !contains("."),

and return no properties in that case.


In this case, I think getNamespace() should return "window" or null. And
getProperty() should continue to return the original string, because I see
it as a property on a global (window) namespace. I've Javadoc'ed this to be
clear

http://codereview.appspot.com/4175057/
>

Updated patch.

[fargo, 2011-02-23 19:36:40]

On Wed, Feb 23, 2011 at 11:24 AM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Tue, Feb 22, 2011 at 5:07 PM, <johnfargo@gmail.com> wrote:
>
>>
>>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>> File
>>
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java
>> (right):
>>
>>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:65:
>> static final String EXPORT_JS_FUNCTION =
>> let's put all export code into a feature ("exportjs") of its own, so we
>> can write JsUnit tests for it, include it in dependency resolution, etc.
>>
>
> Agree on testability, but where to put the dependency resolution? We can do
> it in JsHandler (if >=1 exports statements, add feature=exportjs to needed
> bundles), but I'm hoping that resolution is better encapsulated in 1 place,
> ie: the compiler should just chew raw JS string (not deal with feature
> augmentation).
>

I agree that the dependency piece is tricky. Let's punt on that for now --
we can still turn the function into a feature and just load it in
AbstractJsCompiler by injecting FeatureRegistry and retrieving the code that
way.


>
>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:66:
>> "function exportGlobalJs(props) {" +
>> by my read, exportLocalJs can do everything exportGlobalJs can -- just
>> without a "props" argument, ie:
>>
>> exportLocalJs("gadgets", [ gadgets ]);
>>
>> export-generation code should be responsible for generating these. It
>> emits a "props" argument only when there was a top-level ns/object.
>>
>
> Right. Removed exportGlobalJs.
>
>
>>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:91:
>> "  } else if (typeof base === 'function') {" +
>> As noted in a separate thread, we need to augment this function to
>> support prototype-style objects.
>
> The function should stop at token "prototype", then replace the prefix
>> to that point with wrapper function like:
>> var exportedFn = function() {
>>  this.constructor = base.constructor;
>>  this.__proto__ = base.__proto__; // maybe
>>  exportProps(this);
>> };
>>
>> But, it seems to work fine as-is --
>
> // given ...
> gadgets.lib4 = function() {};
> gadgets.lib4.prototype.unquoted = function() { return "foo" };
> // gets compiled to ...
> a.b.prototype.a = function $f() { return"foo" };
> d("gadgets.lib4.prototype", [a, a.b, a.b.prototype], {a:"unquoted"}); //
> exportLocalJs
> // which results ...
> gadgets.lib4.prototype.a = $f
> gadgets.lib4.prototype.unquoted = $f
>
> This is what we expect, no?  The logic sounds sane to me, given the input
> "gadgets.lib4.prototype", after traversal, base becomes the prototype, which
> is of type 'object' (not type 'function'), which then exportProps() like
> normal (on the prototype). So, I don't think we need to stop at token
> 'prototype'.
>

Interesting -- I suppose that does seem reasonable. Cool!

BTW this reminds me, we should slightly update the exportJs function to
ignore exporting syms on objects where they already exist. In other words,
change this line:
if (props.hasOwnProperty(prop)) {

...to:
if (props.hasOwnProperty(prop) && root.hasOwnProperty(prop)) {

...so that we don't break objects created with quoted returns ie:
return {
  'foo': function() { }
};

exportJs("object.foo", [ object ], { foo: "foo" }); --> compiled to
something like { q: "foo" } where the object doesn't actually have property
"q".


>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:175:
>> result.append("exportLocalJs('").append(input.namespace).append("',[");
>> nit: public static final String for the function name
>>
>
> Done.
>
>
>>
>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:195:
>> private String getNamespace(String symbol) {
>> as noted above, you could just return the full string if !contains("."),
>
> and return no properties in that case.
>
>
> In this case, I think getNamespace() should return "window" or null. And
> getProperty() should continue to return the original string, because I see
> it as a property on a global (window) namespace. I've Javadoc'ed this to be
> clear
>

I suppose that's fine -- it just means that you have to add some
special-case logic in your exportJs()-generating code to accommodate
scenarios in which there is no namespace.


>
> http://codereview.appspot.com/4175057/
>>
>
> Updated patch.
>
>

[mhermanto, 2011-02-23 22:46:24]

On Wed, Feb 23, 2011 at 11:36 AM, John Hjelmstad <fargo@google.com> wrote:

> On Wed, Feb 23, 2011 at 11:24 AM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>>
>>
>> On Tue, Feb 22, 2011 at 5:07 PM, <johnfargo@gmail.com> wrote:
>>
>>>
>>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>> File
>>>
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java
>>> (right):
>>>
>>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:65:
>>> static final String EXPORT_JS_FUNCTION =
>>> let's put all export code into a feature ("exportjs") of its own, so we
>>> can write JsUnit tests for it, include it in dependency resolution, etc.
>>>
>>
>> Agree on testability, but where to put the dependency resolution? We can
>> do it in JsHandler (if >=1 exports statements, add feature=exportjs to
>> needed bundles), but I'm hoping that resolution is better encapsulated in 1
>> place, ie: the compiler should just chew raw JS string (not deal with
>> feature augmentation).
>>
>
> I agree that the dependency piece is tricky. Let's punt on that for now --
> we can still turn the function into a feature and just load it in
> AbstractJsCompiler by injecting FeatureRegistry and retrieving the code that
> way.
>

Done. This does, however, touch more code -- FeatureRegistry, GadgetContext.


>
>
>>
>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:66:
>>> "function exportGlobalJs(props) {" +
>>> by my read, exportLocalJs can do everything exportGlobalJs can -- just
>>> without a "props" argument, ie:
>>>
>>> exportLocalJs("gadgets", [ gadgets ]);
>>>
>>> export-generation code should be responsible for generating these. It
>>> emits a "props" argument only when there was a top-level ns/object.
>>>
>>
>> Right. Removed exportGlobalJs.
>>
>>
>>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:91:
>>> "  } else if (typeof base === 'function') {" +
>>> As noted in a separate thread, we need to augment this function to
>>> support prototype-style objects.
>>
>> The function should stop at token "prototype", then replace the prefix
>>> to that point with wrapper function like:
>>> var exportedFn = function() {
>>>  this.constructor = base.constructor;
>>>  this.__proto__ = base.__proto__; // maybe
>>>  exportProps(this);
>>> };
>>>
>>> But, it seems to work fine as-is --
>>
>> // given ...
>> gadgets.lib4 = function() {};
>> gadgets.lib4.prototype.unquoted = function() { return "foo" };
>> // gets compiled to ...
>> a.b.prototype.a = function $f() { return"foo" };
>> d("gadgets.lib4.prototype", [a, a.b, a.b.prototype], {a:"unquoted"}); //
>> exportLocalJs
>> // which results ...
>> gadgets.lib4.prototype.a = $f
>> gadgets.lib4.prototype.unquoted = $f
>>
>> This is what we expect, no?  The logic sounds sane to me, given the input
>> "gadgets.lib4.prototype", after traversal, base becomes the prototype, which
>> is of type 'object' (not type 'function'), which then exportProps() like
>> normal (on the prototype). So, I don't think we need to stop at token
>> 'prototype'.
>>
>
> Interesting -- I suppose that does seem reasonable. Cool!
>
> BTW this reminds me, we should slightly update the exportJs function to
> ignore exporting syms on objects where they already exist. In other words,
> change this line:
> if (props.hasOwnProperty(prop)) {
>
> ...to:
> if (props.hasOwnProperty(prop) && root.hasOwnProperty(prop)) {
>
> ...so that we don't break objects created with quoted returns ie:
> return {
>   'foo': function() { }
> };
>
> exportJs("object.foo", [ object ], { foo: "foo" }); --> compiled to
> something like { q: "foo" } where the object doesn't actually have property
> "q".
>
> Done.


>
>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:175:
>>> result.append("exportLocalJs('").append(input.namespace).append("',[");
>>> nit: public static final String for the function name
>>>
>>
>> Done.
>>
>>
>>>
>>>
http://codereview.appspot.com/4175057/diff/1/java/gadgets/src/main/java/org/a...
>>>
>>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/js/AbstractJsCompiler.java:195:
>>> private String getNamespace(String symbol) {
>>> as noted above, you could just return the full string if !contains("."),
>>
>> and return no properties in that case.
>>
>>
>> In this case, I think getNamespace() should return "window" or null. And
>> getProperty() should continue to return the original string, because I see
>> it as a property on a global (window) namespace. I've Javadoc'ed this to be
>> clear
>>
>
> I suppose that's fine -- it just means that you have to add some
> special-case logic in your exportJs()-generating code to accommodate
> scenarios in which there is no namespace.
>
>
>>
>> http://codereview.appspot.com/4175057/
>>>
>>
>> Updated patch.
>>
>>
Updated patch.