Add JavaScriptSourceNode to represent runtime libraries (fixes #85)

Add JavaScriptSourceNode to represent runtime libraries (fixes #85)

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/599bf2dcd231b667c44f9ab8ac9a29d608c0f673

[Siggi Cherem (dart-lang), 2015-03-07 02:24:49]

sigmund@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2015-03-09 16:12:42]

implementation LGTM, with some comments about comments/style

https://codereview.chromium.org/986723002/diff/1/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/986723002/diff/1/lib/devc.dart#newcode127
lib/devc.dart:127: // JavaScriptSourceNodes are pieces of the dev_compiler
runtime which are
Is this necessarily the case? What about other .js files, e.g. written by hand,
or pulled in from a package manager like NPM/Bower?

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dart
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dar...
lib/src/dependency_graph.dart:114: Set<JavaScriptSourceNode> runtimeDeps = new
Set<JavaScriptSourceNode>();
Are we moving to a style where we duplicate the full type?
I've been trying to write this as:

    final runtimeDeps = new Set<JavaScriptSourceNode>();

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dar...
lib/src/dependency_graph.dart:339: if (n is JavaScriptSourceNode) return false;
eventually we may want to follow ES6 imports?

https://codereview.chromium.org/986723002/diff/1/test/dependency_graph_test.dart
File test/dependency_graph_test.dart (right):

https://codereview.chromium.org/986723002/diff/1/test/dependency_graph_test.d...
test/dependency_graph_test.dart:556: |-- harmony_feature_check.js
not related to this CL, but ... I'm not sure we want to inject this
automatically. Feature testing should be up to the server, so it can skip this
file on newer browsers.

[Siggi Cherem (dart-lang), 2015-03-09 17:31:39]

https://codereview.chromium.org/986723002/diff/1/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/986723002/diff/1/lib/devc.dart#newcode127
lib/devc.dart:127: // JavaScriptSourceNodes are pieces of the dev_compiler
runtime which are
On 2015/03/09 16:12:41, John Messerly wrote:
> Is this necessarily the case? What about other .js files, e.g. written by
hand,
> or pulled in from a package manager like NPM/Bower?

True, good point. I'll adjust the comment. FYI in a follow up CL I'm changing
this to "ResourceSourceNode, since it can be other things too.

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dart
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dar...
lib/src/dependency_graph.dart:114: Set<JavaScriptSourceNode> runtimeDeps = new
Set<JavaScriptSourceNode>();
On 2015/03/09 16:12:41, John Messerly wrote:
> Are we moving to a style where we duplicate the full type?
> I've been trying to write this as:
> 
>     final runtimeDeps = new Set<JavaScriptSourceNode>();

Good question - I would prefer not to as well. But I keep debating on what's
best to do if the field is not final.

In this case I think I can switch it to be final, but in the case of Script
below, I'm not sure what's best to do. I'm inclined to change it to:

  Set<DartSourceNode> scripts = new Set();

thoughts?

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dar...
lib/src/dependency_graph.dart:339: if (n is JavaScriptSourceNode) return false;
On 2015/03/09 16:12:41, John Messerly wrote:
> eventually we may want to follow ES6 imports?

we totally could if it comes to it.

https://codereview.chromium.org/986723002/diff/1/test/dependency_graph_test.dart
File test/dependency_graph_test.dart (right):

https://codereview.chromium.org/986723002/diff/1/test/dependency_graph_test.d...
test/dependency_graph_test.dart:556: |-- harmony_feature_check.js
On 2015/03/09 16:12:41, John Messerly wrote:
> not related to this CL, but ... I'm not sure we want to inject this
> automatically. Feature testing should be up to the server, so it can skip this
> file on newer browsers.

True, I've opened #87 to track

[Jennifer Messerly, 2015-03-09 17:34:18]

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dart
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/986723002/diff/1/lib/src/dependency_graph.dar...
lib/src/dependency_graph.dart:114: Set<JavaScriptSourceNode> runtimeDeps = new
Set<JavaScriptSourceNode>();
On 2015/03/09 17:31:39, Siggi Cherem (dart-lang) wrote:
> On 2015/03/09 16:12:41, John Messerly wrote:
> > Are we moving to a style where we duplicate the full type?
> > I've been trying to write this as:
> > 
> >     final runtimeDeps = new Set<JavaScriptSourceNode>();
> 
> Good question - I would prefer not to as well. But I keep debating on what's
> best to do if the field is not final.
> 
> In this case I think I can switch it to be final, but in the case of Script
> below, I'm not sure what's best to do. I'm inclined to change it to:
> 
>   Set<DartSourceNode> scripts = new Set();
> 
> thoughts?

yeah, seems fine to me. Could also be "var" I suppose (not sure if we have plans
to distinguish the two).

fwiw, at least until we can self host, I've seen it useful to have a reified
generic type when using js_ast, e.g. to catch expressions being placed in a
statement list.

[Siggi Cherem (dart-lang), 2015-03-09 18:33:50]

Committed patchset #2 (id:20001) manually as
599bf2dcd231b667c44f9ab8ac9a29d608c0f673 (presubmit successful).