Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Output Closure annotations when --closure #286

Merged
merged 1 commit into from
Sep 2, 2015
Merged

Output Closure annotations when --closure #286

merged 1 commit into from
Sep 2, 2015

Conversation

ochafik
Copy link
Contributor

@ochafik ochafik commented Aug 8, 2015

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@ochafik ochafik force-pushed the closure-annotations branch from 53f6327 to b2b6067 Compare August 8, 2015 03:03
@jmesserly
Copy link
Contributor

Really nice work. CC @vsmenon and @leafpetersen for additional thoughts.

Main thing I'm wondering is what to do about precompiled SDK JS files. I don't think we want those to use --closure but not sure what alternative to suggest. Related to #257 (compiling against a different SDK than we run with) and #223 (incremental compile). The precompiled SDK is kind of a hack to avoid SDK build time, but it causes problems like this one.

I guess we have a related problem with hand coded libraries. They turned out to be a lot bigger than I was expecting. Since we haven't used the JS builtin style of dart2js, we don't have ability to apply compiler transforms to them (like 6->5 lowering, closure annotations, minify, etc.)

Anyway, maybe we can discuss this further here or have a design meeting offline. Cheers, - John

@ochafik ochafik force-pushed the closure-annotations branch 2 times, most recently from 4f0bcf2 to 906fff0 Compare August 25, 2015 23:46
@ochafik
Copy link
Contributor Author

ochafik commented Aug 25, 2015

Hey John, I've added a bit of tests + dropped the --closure flag from build_sdk.sh. Instead, for now, people who want to play with the closure backend will have to rebuild the sdk with ./tool/build_sdk.sh --closure (when things are production-strength, we can talk again about that smart 1-time-cost + caching).

I don't have an answer for hard coded libraries, except that... what prevents us from writing them in Dart? (maybe some annotation could trigger a dart:js pass-through mode, which would allow to express near-complete JS semantics?).

@jmesserly
Copy link
Contributor

I don't have an answer for hard coded libraries, except that... what prevents us from writing them in Dart? (maybe some annotation could trigger a dart:js pass-through mode, which would allow to express near-complete JS semantics?).

CC @vsmenon @leafpetersen for thoughts?

The dart2js style is to have them in a private dart: library and with "JS" snippets inside method/function bodies.

The context here: if we have multiple output modes (ES6, Closure, TypeScript, maybe something else in the future? also different JS module systems) it might be easier to maintain our runtime helpers if they're in a Dart library vs a .js file.

jmesserly
jmesserly reviewed Aug 25, 2015
View reviewed changes
lib/src/closure/closure_annotation.dart
@@ -0,0 +1,127 @@
// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2015 in the various new files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, done.

@jmesserly
Copy link
Contributor

this LGTM 👍 ... @leafpetersen would you be interested in reviewing the type annotation code?

@jmesserly
Copy link
Contributor

aside: this is really great looking code! I really like how it reads and is factored

@ochafik ochafik force-pushed the closure-annotations branch from 67734f7 to 3760837 Compare August 26, 2015 16:25
@ochafik
Copy link
Contributor Author

ochafik commented Aug 26, 2015

Thanks a lot John!

vsmenon
vsmenon reviewed Aug 26, 2015
View reviewed changes
lib/src/closure/closure_codegen.dart
import 'package:analyzer/src/generated/resolver.dart' show TypeProvider;

/// Mixin that can generate [ClosureAnnotation]s for Dart elements and types.
abstract class ClosureCodegen {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: ClosureCodegen -> ClosureAnnotator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ochafik ochafik force-pushed the closure-annotations branch 4 times, most recently from e58a5f9 to 7e3c472 Compare August 27, 2015 15:04
@ochafik ochafik force-pushed the closure-annotations branch from 7e3c472 to 7d3c0b0 Compare August 27, 2015 15:30
@leafpetersen
Copy link
Contributor

LGTM.

ochafik added a commit that referenced this pull request Sep 2, 2015
@ochafik
Merge pull request #286 from dart-lang/closure-annotations
ce6e9cb 
Output Closure annotations when --closure (very experimental feature)
@ochafik ochafik merged commit ce6e9cb into master Sep 2, 2015
@ochafik ochafik mentioned this pull request Sep 8, 2015
Open
@ochafik ochafik deleted the closure-annotations branch December 15, 2015 12:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ochafik @googlebot @jmesserly @leafpetersen @vsmenon