-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
Can you take a look before I go an update all the goldens. |
_emitGoogRequireSupport(namespace, isDefault ? symbol.getName() : namespace); | ||
} | ||
|
||
private void _emitGoogRequireSupport(String namespace, String closureNamespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No _
in method names in Java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Approach looks good generally. |
b73665c
to
acdd5f5
Compare
@@ -0,0 +1,4 @@ | |||
/** @provideGoog */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to work.
When debugging compilerInput.getProvides()
returns 0 provides for this file. If I understand @provideGoog properly, it should be providing goog.provide
as there is not other method to provide that during bootstrapping.
@MatrixFrog, anything obviously wrong with this approach. I am trying to replicate base.js
so that the Closure compiler is aware of goog.require
as a provided function symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually see any special handling for the provideGoog annotation in the compiler. As far as I can tell that annotation is understood by some other (Google-internal) tools but not by the compiler itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank Tyler! After some poking around (basically taking a real base.js and bisecting) turns out the special part that makes goog.require
persist as a provided symbol is var COMPILED = false;
.
@mprobst PTAL, before I update the goldens?
if (namespace.equals("goog")) return; | ||
emitNamespaceBegin("goog"); | ||
String qualifiedClosureNamespace = INTERNAL_NAMESPACE + "." + closureNamespace; | ||
emit("function require(name: '" + closureNamespace + "'): typeof " + qualifiedClosureNamespace + ";"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add a comment here pointing out the TypeScript overloading based on string arguments, otherwise this is a bit mistifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
LGTM, except a bit the "cannot use as type" concern, but that's unrelated to the code here. |
Closes #113