-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat(typescript): Add references to the new typescript AST library #5066
Conversation
public addImport(reference: Reference): void { | ||
if (reference.module != null) { | ||
if (reference.module.defaultExport ?? false) { | ||
const existing = this.defaultImports[reference.module.moduleName]; | ||
if (existing == null) { | ||
this.defaultImports[reference.module.moduleName] = reference; | ||
} else if (existing.name !== reference.name) { | ||
throw new Error( | ||
`Cannot have multiple default imports for module ${reference.module.moduleName}: ` + | ||
`got ${reference.name} but already had ${existing.name}` | ||
); | ||
} | ||
} | ||
this.imports[reference.module.moduleName] ??= []; | ||
const moduleImports = this.imports[reference.module.moduleName]; | ||
if (moduleImports != null) { | ||
const names = moduleImports.map((import_) => import_.name); | ||
if (!names.includes(reference.name)) { | ||
moduleImports.push(reference); | ||
} | ||
} | ||
} | ||
} |
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.
Thoughts on splitting this up a bit to reduce nesting? It's otherwise a little hard to follow at first glance given that there's a variety of conditions. What about something along the lines of the following?
public addImport(reference: Reference): void {
if (reference.module == null) {
return;
}
if (reference.module.defaultExport) {
this.addDefaultExportOrThrow({ reference, moduleName: reference.module.name } );
return;
}
const references = this.imports[reference.module.moduleName] ??= [];
if (!references.includes(reference.name)) {
references.push(reference);
}
}
private addReferenceOrThrow({ reference, moduleName }: { reference: Reference; moduleName: string; }): void {
if (this.defaultImports[moduleName] != null) {
throw new Error("Internal error; cannot have multiple default imports for module ...");
}
this.defaultImports[moduleName] = reference;
}
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
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.
Split it up into the different conditions
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.
Ended up looking quite different than to this because I added stars
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.
The functionality looks great, so stamping as-is, but I think we can improve code readability a bit. I trust you to make edits as you see fit, and we can go from there!
if (named.length > 0 || defaultImport != null || starImportAlias != null) { | ||
result += "import"; | ||
if (defaultImport != null) { | ||
result += ` ${defaultImport.name}`; | ||
} | ||
if (named.length > 0) { | ||
for (const ref of named.slice(0, -1)) { | ||
stringifiedNonDefault += `${ref.name}, `; | ||
} | ||
const lastRef = named[named.length - 1]; | ||
// Need for eslint; lastRef will not be null because length > 0 | ||
if (lastRef != null) { | ||
stringifiedNonDefault += `${lastRef.name}`; | ||
} | ||
if (defaultImport != null) { | ||
result += ","; | ||
} | ||
result += ` { ${stringifiedNonDefault} }`; | ||
} | ||
if (starImportAlias != null) { | ||
if (defaultImport != null || named.length > 0) { | ||
result += ", "; | ||
} | ||
result += ` * as ${starImportAlias}`; | ||
} | ||
result += ` from "${module}";\n`; | ||
} |
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.
We might be able to reduce nesting here too if we rearrange things a bit.
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.
What about something like this? We basically have a function that can turn a single module + multiple imports into statements. each statement is just an import
type Import = DefaultImport | NamedImport | StartImport;
export function stringifyImports(): string[] {
const importsByModule: Record<Module, Import[]> = getImportsByModule();
const statements = Object.entries(importsByModule).flatMap((module, imports) => {
return generateStatementsForImports(module, imports);
});
return statements;
}
export function generateStatementsForImports(module, imports): string[] {
const statements = []
const nonStartImport: string[] = [];
for (const import of imports) {
switch (import.type) {
case "named":
nonStartImportStatement.push(named.value);
case "startImport":
statements.push(`import * as ${import.alias} from ${module}`)
}
}
if (nonStarImport.length > 0) {
statements.push(`import ${nonStartImport.join(", ")} from ${module}`)
}
return statements;
}
private addStarImport(reference: Reference): void { | ||
if (reference.importFrom?.type === "star") { | ||
const existing = this.imports[reference.importFrom.moduleName]; | ||
if (existing != null) { | ||
const existingNamed = existing.filter((e) => e.importFrom?.type === "named"); | ||
if (existingNamed.length > 0) { | ||
throw new Error( | ||
`Cannot add non-named import ${reference.name} because named` + | ||
` imports ${existingNamed.map((e) => e.name)} already exist` | ||
); | ||
} | ||
} | ||
const moduleForAlias = this.starImportAliasesInverse[reference.importFrom.starImportAlias]; | ||
if (moduleForAlias != null && moduleForAlias !== reference.importFrom.moduleName) { | ||
throw new Error( | ||
`Attempted to use alias ${reference.importFrom.starImportAlias} for more than one ` + | ||
"module in the same file" | ||
); | ||
} | ||
const existingAlias = this.starImportAliases[reference.importFrom.moduleName]; | ||
if (existingAlias == null) { | ||
this.starImportAliases[reference.importFrom.moduleName] = reference.importFrom.starImportAlias; | ||
this.starImportAliasesInverse[reference.importFrom.starImportAlias] = reference.importFrom.moduleName; | ||
} else if (existingAlias != null && existingAlias !== reference.importFrom.starImportAlias) { | ||
throw new Error( | ||
"Cannot have more than one alias for non-named imports from a module: " + | ||
`got ${reference.importFrom.starImportAlias} but already have ${existingAlias}.` | ||
); | ||
} | ||
} | ||
} |
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.
There's a fair amount of nesting in these functions - I'd try to reduce nesting as much as possible and split these out into separate functions so that it's a little easier to read and debug later on.
I think we can apply an approach similar to the one outlined in this comment. For example, reversing the if
condition at the top would reduce the entire function nesting:
if (reference.importFrom?.type !== "star") {
return;
}
...
In summary, I'd try to:
- Reduce nesting and return early as much as possible. Although this guide is focused on Go, the same ideas apply here.
- Add
*OrThrow
functions to handle error cases. - With the restructured control flow, simplify the conditions used in each
if
down to a minimum.
@@ -0,0 +1,47 @@ | |||
import { AstNode, Writer } from "./core"; | |||
|
|||
export declare namespace Reference { |
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.
Nice, this AstNode looks great.
} | ||
} | ||
} | ||
// TODO: Need to be able to resolve conflicts instead of throwing |
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.
Discussed offline -
We'll need to build this so that it's resilient to import conflicts. For example, consider the following:
export { File } from "./acme/foo/File";
export { File } from "./acme/bar/File";
We don't want to throw in these cases - the generator should handle the alias and assign it as needed.
export { File } from "./acme/foo/File";
export { File as File_ } from "./acme/bar/File";
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.
Discussed offline - there's some things we'll want to revisit for the import management, but this is a great first step in the right direction. We can handle those changes in a follow-up. Well done!
This PR adds references with imports to the typescript AST, which allows for implementing further nodes like function calls, class instantiation, etc.