Incremental JS loading

Be able to serve partial JS, given that some libs have been loaded. Idea: accumulate exports and externs of loaded JS libs (and their transitively-dependent libs) for externs to requested JS libs (and their transitively-dependent libs, minus explicitly-requested JS libs).

[johnfargo, 2011-04-08 03:35:37]

Pretty small nits all told -- code looks clean, good test coverage.

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

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/GetJsContentProcessor.java:97:
"Unknown feature(s): " + UNKNOWN_FEATURE_ERR.join(unsupported));
One could make an argument that this error is unnecessary in the &loaded= param
case. Let's keep it as-is but at least distinguish the error text between one
case and the other.

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

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java:35:
private List<JsContent> jsCode;
curious, why the move to List?

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java
(right):

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java:70:
eq("extern3;\n" + "extern4;\n"))).andReturn(outputResponse);
nit: could make the JsResponseBuilder joiner public (still static final) to bind
its logic w/ this test.

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java
(right):

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:112:
List<String> reqLibs = ImmutableList.of("feature1", "feature2");
nit: <String> unnecessary in rvalue

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:139:
FeatureBundle bundle = mockBundle("feature", null, null,
Lists.newArrayList(resource1, resource2));
nit: >100char

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:200:
private void checkResponse(boolean proxyCacheable, int expectedTtl, String
jsString, String externsString) {
nit:>100char

[mhermanto, 2011-04-08 10:11:02]

On Thu, Apr 7, 2011 at 8:35 PM, <johnfargo@gmail.com> wrote:

> Pretty small nits all told -- code looks clean, good test coverage.
>
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/GetJsContentProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/GetJsContentProcessor.java:97:
> "Unknown feature(s): " + UNKNOWN_FEATURE_ERR.join(unsupported));
> One could make an argument that this error is unnecessary in the
> &loaded= param case. Let's keep it as-is but at least distinguish the
> error text between one case and the other.
>

Done.

http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java
> (right):
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java:35:
> private List<JsContent> jsCode;
> curious, why the move to List?
>

Just making use of the interface more, and looks in-line with other
List-based members.


>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
> File
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java
> (right):
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java:70:
> eq("extern3;\n" + "extern4;\n"))).andReturn(outputResponse);
> nit: could make the JsResponseBuilder joiner public (still static final)
> to bind its logic w/ this test.
>

Hm, exposing Joiner alone is not enough, still care about the last delim.
Agree on abstracting this logic, but don't feel good exposing static
function concatExterns() when the job of the class is to build a response.
I'd leave as is, unless u have strong opinions.


>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
> File
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java
> (right):
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:112:
> List<String> reqLibs = ImmutableList.of("feature1", "feature2");
> nit: <String> unnecessary in rvalue
>

Done.

>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:139:
> FeatureBundle bundle = mockBundle("feature", null, null,
> Lists.newArrayList(resource1, resource2));
> nit: >100char
>

Done.


>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:200:
> private void checkResponse(boolean proxyCacheable, int expectedTtl,
> String jsString, String externsString) {
> nit:>100char
>
>
Done.

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

[johnfargo, 2011-04-08 14:11:07]

Lgtm++

Test stuff is just minor easy maintenance. Stoked to see this in!

On Friday, April 8, 2011, Michael Hermanto <mhermanto@gmail.com> wrote:
>
>
> On Thu, Apr 7, 2011 at 8:35 PM,  <johnfargo@gmail.com> wrote:
>
>
> Pretty small nits all told -- code looks clean, good test coverage.
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/GetJsContentProcessor.java
>
> (right):
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/GetJsContentProcessor.java:97:
> "Unknown feature(s): " + UNKNOWN_FEATURE_ERR.join(unsupported));
> One could make an argument that this error is unnecessary in the
> &loaded= param case. Let's keep it as-is but at least distinguish the
> error text between one case and the other.
>
> Done.
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java
>
> (right):
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/main/java/org/a...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java:35:
> private List<JsContent> jsCode;
> curious, why the move to List?
>
> Just making use of the interface more, and looks in-line with other List-based
members.
>
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java
>
> (right):
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/CompilationProcessorTest.java:70:
> eq("extern3;\n" + "extern4;\n"))).andReturn(outputResponse);
> nit: could make the JsResponseBuilder joiner public (still static final)
> to bind its logic w/ this test.
>
> Hm, exposing Joiner alone is not enough, still care about the last delim.
Agree on abstracting this logic, but don't feel good exposing static function
concatExterns() when the job of the class is to build a response. I'd leave as
is, unless u have strong opinions.
>
>
 http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java
>
> (right):
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:112:
> List<String> reqLibs = ImmutableList.of("feature1", "feature2");
> nit: <String> unnecessary in rvalue
>
> Done.
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:139:
> FeatureBundle bundle = mockBundle("feature", null, null,
> Lists.newArrayList(resource1, resource2));
> nit: >100char
>
> Done.
>
>
http://codereview.appspot.com/4376045/diff/1/java/gadgets/src/test/java/org/a...
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/GetJsContentProcessorTest.java:200:
> private void checkResponse(boolean proxyCacheable, int expectedTtl,
> String jsString, String externsString) {
> nit:>100char
>
> Done.
>
>
>
> http://codereview.appspot.com/4376045/
>
>