Skip to content

Feat#Provide a quick-fix for non-exported imports #72

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.

Sign up for GitHub

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

Open
Richard-Lynch wants to merge 12 commits into
base: oss-hackathon-37440
Choose a base branch
from
Open

Feat#Provide a quick-fix for non-exported imports #72

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
99c6de4
Add diagnostics messages
Jun 16, 2022
ffa9cf2
Update `insertExportModifier` to support `VariableDeclarationList`
Jun 16, 2022
f3ec897
Add `fixImportNonExportedMember`
Jun 16, 2022
c68ae3b
Add tests for `fixImportNonExportedMember`
Jun 16, 2022
5cc5f52
Ensure that `export type` isn't clobbered
Jun 23, 2022
04f1066
Add test for `export type`
Jun 23, 2022
a71c68f
Add more tests
Jun 23, 2022
700141e
Ensure `export {...} from` is not used for the export
Jun 23, 2022
2225781
Simplify main logic flow
Jun 23, 2022
5ff147b
Add test for `export * as`
Jun 23, 2022
6dba610
Update diagnostics message
Jun 24, 2022
ec6324a
Refactor tests
Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add fixImportNonExportedMember 
Signed-off-by: Richard Lynch <rlynch79@bloomberg.net>
  • Loading branch information
Richard Lynch committed Jun 16, 2022
commit f3ec897e2f8860948e5f5db55dc122e98187bf71
229 changes: 229 additions & 0 deletions src/services/codefixes/fixImportNonExportedMember.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
/* @internal */
namespace ts.codefix {
const fixId = "importNonExportedMember";

const errorCodes = [
Diagnostics.Module_0_declares_1_locally_but_it_is_not_exported.code,
];

registerCodeFix({
errorCodes,

getCodeActions(context) {
const { sourceFile } = context;

const info = getInfo(sourceFile, context, context.span.start);

if (!info || info.originSourceFile.isDeclarationFile) {
return undefined;
}

const changes = textChanges.ChangeTracker.with(context, (t) =>
doChange(t, info.originSourceFile, info.node)
);

return [
createCodeFixAction(
/*fixName*/ fixId,
changes,
/*description*/ [
Diagnostics.Export_0_from_module_1,
info.node.text,
showModuleSpecifier(info.importDecl),
],
fixId,
/*fixAllDescription*/ Diagnostics.Add_all_missing_exports
),
];
},

fixIds: [fixId],

getAllCodeActions: (context) =>
codeFixAll(context, errorCodes, (changes, diag) => {
const info = getInfo(diag.file, context, diag.start);

if (info) {
doChange(changes, info.originSourceFile, info.node);
}
}),
});

interface Info {
readonly node: Identifier;

readonly importDecl: ImportDeclaration;

readonly originSourceFile: SourceFile;
}

function getInfo(
sourceFile: SourceFile,
context: CodeFixContext | CodeFixAllContext,
pos: number
): Info | undefined {
const node = getTokenAtPosition(sourceFile, pos);

if (!isIdentifier(node)) {
return;
}

const importDecl = findAncestor(node, isImportDeclaration);

if (!importDecl || !isStringLiteralLike(importDecl.moduleSpecifier)) {
return undefined;
}

const resolvedModule = getResolvedModule(
sourceFile,
/*moduleName*/ importDecl.moduleSpecifier.text,
/*mode*/ undefined
);

if (!resolvedModule) {
return undefined;
}

const originSourceFile = context.program.getSourceFile(
resolvedModule.resolvedFileName
);

if (!originSourceFile) {
return undefined;
}

return { node, importDecl, originSourceFile };
}

function getNamedExportDeclaration(
sourceFile: SourceFile
): ExportDeclaration | undefined {
let namedExport;

for (const statement of sourceFile.statements) {
if (
isExportDeclaration(statement) &&
statement.exportClause &&
isNamedExports(statement.exportClause)
Copy link
Copy Markdown

@mkubilayk mkubilayk Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should skip re-export statements here: export { a } from "./something";.

Copy link
Copy Markdown
Author

@Richard-Lynch Richard-Lynch Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup nice, added

) {
namedExport = statement;
}
}

return namedExport;
}

function compareIdentifiers(s1: Identifier, s2: Identifier) {
return compareStringsCaseInsensitive(s1.text, s2.text);
}

function sortSpecifiers(
specifiers: ExportSpecifier[]
): readonly ExportSpecifier[] {
return stableSort(specifiers, (s1, s2) =>
compareIdentifiers(
s1.propertyName || s1.name,
s2.propertyName || s2.name
)
);
}

const isVariableDeclarationListWith1Element = (
node: Node
): node is VariableDeclarationList =>
!(isVariableDeclarationList(node) && node.declarations.length !== 1);

function doChange(
changes: textChanges.ChangeTracker,
sourceFile: SourceFile,
node: Identifier
): void {
const moduleSymbol = sourceFile.localSymbol ?? sourceFile.symbol;

const localSymbol = moduleSymbol.valueDeclaration?.locals?.get(
node.escapedText
);

if (localSymbol === undefined) {
return;
}

// Node we need to export is a function
if (isFunctionSymbol(localSymbol)) {
const node = localSymbol.valueDeclaration;

if (node === undefined) {
return;
}

if (!isDeclarationStatement(node) && !isVariableStatement(node)) {
return;
}

return changes.insertExportModifier(sourceFile, node);
}
// Node we need to export is a variable declaration
else if (
localSymbol.valueDeclaration &&
isVariableDeclarationListWith1Element(
localSymbol.valueDeclaration.parent
)
) {
const node = localSymbol.valueDeclaration.parent;

return changes.insertExportModifier(sourceFile, node);
}
Copy link
Copy Markdown

@mkubilayk mkubilayk Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need special handling for class declarations as well?

Copy link
Copy Markdown
Author

@Richard-Lynch Richard-Lynch Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class is captured/handled with isDeclarationStatement but I've added some tests and simplified that logic


// In all other cases => the node is a variable, and should be exported via `export {a}`
const namedExportDeclaration = getNamedExportDeclaration(sourceFile);

// If there is an existing export
if (
namedExportDeclaration?.exportClause &&
isNamedExports(namedExportDeclaration.exportClause)
Copy link
Copy Markdown

@mkubilayk mkubilayk Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check if this is a type-only export? My understanding is that this would change:

export type { existingThing };

to

export { existingThing, newThing };

which may not be desired.

Copy link
Copy Markdown
Author

@Richard-Lynch Richard-Lynch Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! yes, this is a good test case, adding now

) {
return changes.replaceNode(
sourceFile,
namedExportDeclaration,
factory.updateExportDeclaration(
/*node*/ namedExportDeclaration,
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
/*exportClause*/ factory.updateNamedExports(
/*node*/ namedExportDeclaration.exportClause,
/*elements*/ sortSpecifiers(
namedExportDeclaration.exportClause.elements.concat(
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
)
)
)
),
/*moduleSpecifier*/ undefined,
/*assertClause*/ undefined
)
Copy link
Copy Markdown

@mkubilayk mkubilayk Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
factory.updateExportDeclaration(
/*node*/ namedExportDeclaration,
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
/*exportClause*/ factory.updateNamedExports(
/*node*/ namedExportDeclaration.exportClause,
/*elements*/ sortSpecifiers(
namedExportDeclaration.exportClause.elements.concat(
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
)
)
)
),
/*moduleSpecifier*/ undefined,
/*assertClause*/ undefined
)
factory.updateExportDeclaration(
namedExportDeclaration,
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
factory.updateNamedExports(
namedExportDeclaration.exportClause,
sortSpecifiers(
namedExportDeclaration.exportClause.elements.concat(
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
)
)
)
),
/*moduleSpecifier*/ undefined,
/*assertClause*/ undefined
)

I think some of these arguments are self explanatory.

Copy link
Copy Markdown
Author

@Richard-Lynch Richard-Lynch Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to remove any of these you'd like, but tbh as someone new to the project => they're not :P

Honestly half the pain is having so many positional arguments at all!

);
}
// There is no existing export
else {
return changes.insertNodeAtEndOfScope(
sourceFile,
sourceFile,
factory.createExportDeclaration(
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
/*exportClause*/ factory.createNamedExports([
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
),
]),
Copy link
Copy Markdown

@mkubilayk mkubilayk Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*exportClause*/ factory.createNamedExports([
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
),
]),
factory.createNamedExports([
factory.createExportSpecifier(
/*isTypeOnly*/ false,
/*propertyName*/ undefined,
node
),
]),

/*moduleSpecifier*/ undefined
)
);
}
}
}
3 changes: 2 additions & 1 deletion src/services/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
"codefixes/convertConstToLet.ts",
"codefixes/fixExpectedComma.ts",
"codefixes/fixAddVoidToPromise.ts",
"codefixes/fixImportNonExportedMember.ts",
"refactors/convertExport.ts",
"refactors/convertImport.ts",
"refactors/convertToOptionalChainExpression.ts",
Expand All @@ -134,6 +135,6 @@
"transform.ts",
"shims.ts",
"globalThisShim.ts",
"exportAsModule.ts"
"exportAsModule.ts",
]
}