-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UMD support #7264
UMD support #7264
Conversation
@@ -4604,6 +4609,7 @@ namespace ts { | |||
case SyntaxKind.EnumKeyword: | |||
return parseEnumDeclaration(fullStart, decorators, modifiers); | |||
case SyntaxKind.GlobalKeyword: | |||
return parseModuleDeclaration(fullStart, decorators, modifiers); |
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.
You can let this fall through again.
if (parent.externalModuleIndicator && !parent.isDeclarationFile) { | ||
error(node, Diagnostics.Global_module_exports_may_only_appear_in_declaration_files); | ||
} | ||
} |
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.
Spoke offline with @vladima. Add all of this to the binder.
Any other comments? |
@@ -981,6 +981,10 @@ namespace ts { | |||
return getExternalModuleMember(<ImportDeclaration>node.parent.parent.parent, node); | |||
} | |||
|
|||
function getTargetOfGlobalModuleExportDeclaration(node: GlobalModuleExportDeclaration): Symbol { | |||
return node.parent.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.
can this symbol be merged with anything?
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.
Hrm, yes. You could have a module jquery.d.ts and then an augmentation of it in another file. I assume that has some impact here?
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.
use getMergedSymbol
@vladima added support for |
function getTargetOfGlobalModuleExportDeclaration(node: GlobalModuleExportDeclaration): Symbol { | ||
const moduleSymbol = node.parent.symbol; | ||
if (moduleSymbol && moduleSymbol.exports && moduleSymbol.exports["export="]) { | ||
return moduleSymbol.exports["export="].exportSymbol; |
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.
Does this account for module augmentations? Will you need to get the "merged" symbol in that case?
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 think it does, but I don't know how to make it work either. Any ideas?
Added tests for module augmentation; any other comments? |
else { | ||
const parent = node.parent as SourceFile; | ||
|
||
if (!isExternalModule(<SourceFile>node.parent)) { |
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.
Use parent
here
Can you add a test that uses an |
✔️ |
@RyanCavanaugh TL;DR: Can you add an example that uses |
Fixes #7125