-
Notifications
You must be signed in to change notification settings - Fork 35
[interop] Add support for modules #446
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
base: main
Are you sure you want to change the base?
Conversation
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 is a draft, so some of my comments may not apply. :)
|
||
@override | ||
Directive emit([Options? options]) { | ||
return Directive.export( |
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.
nit: Use =>
instead
String from; | ||
|
||
/// The full path of the actual file being referenced, if any | ||
String? get actualReference => reference.url; |
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.
nit: referenceUrl
instead. actualReference
is ambiguous.
Set<TSNode> get nodes; | ||
|
||
final Set<ParentDeclaration> namespaceDeclarations; |
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.
Should this not be Set<NamespaceDeclaration>
?
.toString()] = decl; | ||
} | ||
|
||
final dependencies = map.entries |
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.
nit: here and elsewhere, prefer:
final dependencies = NodeMap({});
for (final node in map.values) {
dependencies.addAll(_getDependenciesOfDecl(node));
}
|
||
for (final decl in topLevelDeclarations) { | ||
if (decl case final VariableDeclaration variable) { | ||
if (variable.modifier == VariableModifier.$const) { |
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'd avoid the code duplication and let emit
handle this check and then just type-check the result before adding it to the lists, even if it means an extra type-check.
namespaceDeclarations: {}, | ||
nestableDeclarations: {}, | ||
documentation: _parseAndTransformDocumentation(namespace)); | ||
// final outputNamespace = currentNamespaces.isNotEmpty |
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.
nit: stray comments
@@ -1735,9 +1850,24 @@ class Transformer { | |||
} | |||
} else { | |||
final filePathWithoutExtension = file.replaceFirst('.d.ts', ''); | |||
if (p.equals(nameImport, filePathWithoutExtension)) { | |||
final nameIDImport = ID(name: nameImport, type: 'module'); | |||
// print(( |
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.
nit: stray comments
// declared in this file | ||
// if import there and this file, handle this file | ||
print('boo'); |
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.
nit: stray print
// ignore_for_file: library_private_types_in_public_api | ||
// ignore_for_file: non_constant_identifier_names, unnecessary_parenthesis | ||
|
||
@_i1.JS('express') |
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'm not really sure what to do with the library annotation. It'll all depend on where things get exposed I suppose, so this would need the user to put this module in globalThis.express
. Perhaps that is the most logical place for it.
// ignore_for_file: camel_case_types, constant_identifier_names, file_names | ||
// ignore_for_file: non_constant_identifier_names | ||
|
||
@_i1.JS('my-lib') |
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't have -
in @JS
annotations. Prefer converting it to an underscore.
Fixes #424
Fixes #431
This PR adds support for TypeScript Modules, as well as TS Preprocessing, reference modules, and more!