Add support for TypeScript import = require() in web interop generator #511
Conversation
Summary of ChangesHello @sagar-h007, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the web interop generator by introducing comprehensive support for TypeScript's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for TypeScript import = require() declarations, which is a valuable addition for handling CommonJS-style imports within the web interop generator. The implementation correctly dispatches to a new transformation method, _transformImportEqualsAsNamespace, and includes relevant test cases. However, there are a few areas in the new transformation logic that could be improved for robustness and clarity, particularly concerning null safety and error handling.
| final (name: dartName, :id) = currentNamespaces.isEmpty | ||
| ? namer.makeUnique(namespaceName, 'namespace') | ||
| : (name: null, id: null); | ||
|
|
||
| final scopedNamer = ScopedUniqueNamer(); | ||
|
|
||
| final outputNamespace = currentNamespaces.isNotEmpty | ||
| ? currentNamespaces.first | ||
| : NamespaceDeclaration( | ||
| name: namespaceName, | ||
| dartName: dartName, | ||
| id: id!, | ||
| exported: isExported, | ||
| topLevelDeclarations: {}, | ||
| namespaceDeclarations: {}, | ||
| nestableDeclarations: {}, | ||
| documentation: _parseAndTransformDocumentation(importEquals), | ||
| ); |
There was a problem hiding this comment.
The id! operator is used here, but id can be null if currentNamespaces is not empty, as per the preceding ternary operator. This could lead to a runtime Null check operator used on a null value error. When currentNamespaces is not empty, id and dartName should be populated from currentNamespaces.first to ensure they are not null.
final (name: dartName, id: id) = currentNamespaces.isEmpty
? namer.makeUnique(namespaceName, 'namespace')
: (name: currentNamespaces.first.dartName, id: currentNamespaces.first.id);
final scopedNamer = ScopedUniqueNamer();
final outputNamespace = currentNamespaces.isNotEmpty
? currentNamespaces.first
: NamespaceDeclaration(
name: namespaceName,
dartName: dartName,
id: id,
exported: isExported,
topLevelDeclarations: {},
namespaceDeclarations: {},
nestableDeclarations: {},
documentation: _parseAndTransformDocumentation(importEquals),
);| void updateNSInParent() { | ||
| if (parent != null) { | ||
| if (currentNamespaces.isNotEmpty || | ||
| parent.namespaceDeclarations.any((n) => n.name == namespaceName)) { | ||
| parent.namespaceDeclarations.remove(currentNamespaces.first); | ||
| parent.namespaceDeclarations.add(outputNamespace); | ||
| } else { | ||
| outputNamespace.parent = parent; | ||
| parent.namespaceDeclarations.add(outputNamespace); | ||
| } | ||
| } else { | ||
| nodeMap.update( | ||
| outputNamespace.id.toString(), | ||
| (v) => outputNamespace, | ||
| ifAbsent: () => outputNamespace, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The condition currentNamespaces.isNotEmpty || parent.namespaceDeclarations.any((n) => n.name == namespaceName) is problematic. If currentNamespaces is empty, but a matching namespace exists in parent.namespaceDeclarations, the code will attempt to remove(currentNamespaces.first), which will throw an error because currentNamespaces is empty. The logic needs to correctly handle the case where currentNamespaces might be empty but a namespace with the same name exists in the parent's declarations.
void updateNSInParent() {
if (parent != null) {
final existingParentNamespace = parent.namespaceDeclarations.firstWhereOrNull((n) => n.name == namespaceName);
if (existingParentNamespace != null) {
parent.namespaceDeclarations.remove(existingParentNamespace);
parent.namespaceDeclarations.add(outputNamespace);
} else {
outputNamespace.parent = parent;
parent.namespaceDeclarations.add(outputNamespace);
}
} else {
nodeMap.update(
outputNamespace.id.toString(),
(v) => outputNamespace,
ifAbsent: () => outputNamespace,
);
}
}| if (moduleRef.expression.kind != TSSyntaxKind.StringLiteral) { | ||
| print( | ||
| 'WARN: Unsupported import = require() with non-string expression: ' | ||
| '${moduleRef.expression.kind}', | ||
| ); | ||
| // Return empty namespace as fallback | ||
| final (:id, name: dartName) = namer.makeUnique( | ||
| namespaceName, | ||
| 'namespace', | ||
| ); | ||
| return NamespaceDeclaration( | ||
| name: namespaceName, | ||
| dartName: dartName, | ||
| id: id, | ||
| exported: false, | ||
| topLevelDeclarations: {}, | ||
| namespaceDeclarations: {}, | ||
| nestableDeclarations: {}, | ||
| ); |
There was a problem hiding this comment.
The current implementation logs a warning and returns an empty NamespaceDeclaration when moduleRef.expression.kind is not TSSyntaxKind.StringLiteral. While this prevents a crash, returning an empty namespace might lead to silent failures or unexpected behavior downstream if the generated Dart code relies on a non-empty namespace. Consider throwing an UnsupportedError if errorIfUnsupported is true, or providing a more explicit fallback that clearly indicates the namespace is invalid or empty, rather than silently creating an empty one.
| } catch (_) { | ||
| // throws error if no aliased symbol, so ignore | ||
| } |
There was a problem hiding this comment.
Catching all exceptions with catch (_) can hide underlying issues and make debugging difficult. While the comment suggests it's for ignoring errors when no aliased symbol exists, it's generally better to either check for the existence of an aliased symbol explicitly (if the API allows) or catch a more specific exception type if known. If JSError is the expected exception, consider catching it and checking its message for the specific 'no aliased symbol' case.
} on JSError catch (e) {
// Assuming JSError is the specific error thrown when no aliased symbol exists.
// If other errors can occur, they should be handled differently or rethrown.
if (!e.message.contains('No aliased symbol')) {
rethrow;
}
}1 participant
This PR adds support for TypeScript import = require() declarations in the web interop generator. These imports are now recognized and mapped to the existing namespace import flow, allowing symbols to resolve correctly and Dart code to be generated as expected.
What changed
•Added AST handling for ImportEqualsDeclaration.
•Supported ExternalModuleReference with string literal module specifiers.
•Treated import foo = require("bar") the same as a namespace import.
•Ensured namespace symbols are registered correctly for downstream transforms.
•Added tests for import = require() inputs and expected outputs.
Behavior
TypeScript: import foo = require("bar");
Is treated equivalently to: import * as foo from "bar";
Fixes: #438
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.