Skip to content

Feat#Provide a quickfix for non-exported types #37980

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

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -5316,6 +5316,14 @@
"category": "Message",
"code": 90053
},
"Export '{0}' from module '{1}'": {
"category": "Message",
"code": 90054
},
"Add all missing exports": {
"category": "Message",
"code": 90055
},
"Convert function to an ES2015 class": {
"category": "Message",
"code": 95001
Expand Down
110 changes: 110 additions & 0 deletions src/services/codefixes/fixImportNonExportedMember.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/* @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(fixId, changes, [Diagnostics.Export_0_from_module_1, info.node.text, showModuleSpecifier(info.importDecl)], fixId, 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 (node && isIdentifier(node)) {
const importDecl = findAncestor(node, isImportDeclaration);
if (!importDecl || !isStringLiteralLike(importDecl.moduleSpecifier)) {
return undefined;
}
const resolvedModule = getResolvedModule(sourceFile, importDecl.moduleSpecifier.text);
const originSourceFile = resolvedModule && 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)) {
namedExport = statement;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
namedExport = statement;
return statement;

Copy link
Author

Choose a reason for hiding this comment

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

I want to get the last export { } and add the missing export there, so I can't return immediately.

}
}
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) => {
return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name);
});
}

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) {
return;
}
if (isFunctionSymbol(localSymbol)) {
const start = localSymbol.valueDeclaration.pos;
Copy link
Member

Choose a reason for hiding this comment

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

If you're suing pos, that means you're including leading trivia.

changes.insertExportModifierAt(sourceFile, start ? start + 1 : 0);
Copy link
Member

Choose a reason for hiding this comment

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

What exactly is with the start + 1? I don't understand.

Copy link
Author

Choose a reason for hiding this comment

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

I want to add an export identifier in front of non-exported type, you can see the codeFixImportNonExportedMember1.ts, an export added before function bar(), the start there is the position of line break, so need to +1 for start.

Copy link
Member

Choose a reason for hiding this comment

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

The problem you are running into is more about using the pos (a.k.a. the "full start") instead of the start after the trivia (what you'd get when you call getStart(sourceFile)).

As far as I can tell, a test like

/**
 * hello
 */
function bar() {
}

will likely end up with the export inside of the comment..

What happens when you just use insertExportModifier instead of insertExportModifierAt?

return;
}

const current: VariableDeclarationList | Node = localSymbol.valueDeclaration.parent;
if (isVariableDeclarationList(current) && current.declarations.length <= 1) {
const start = localSymbol.valueDeclaration.parent.pos;
changes.insertExportModifierAt(sourceFile, start ? start + 1 : 0);
return;
}

const namedExportDeclaration = getNamedExportDeclaration(sourceFile);
const exportSpecifier = factory.createExportSpecifier(/*propertyName*/ undefined, node);
if (namedExportDeclaration?.exportClause && isNamedExports(namedExportDeclaration.exportClause)) {
const sortedExportSpecifiers = sortSpecifiers(namedExportDeclaration.exportClause.elements.concat(exportSpecifier));
return changes.replaceNode(sourceFile, namedExportDeclaration, factory.updateExportDeclaration(
namedExportDeclaration,
/*decorators*/ undefined,
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
factory.updateNamedExports(namedExportDeclaration.exportClause, sortedExportSpecifiers),
/*moduleSpecifier*/ undefined
));
}
else {
return changes.insertNodeAtEndOfScope(sourceFile, sourceFile, factory.createExportDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
/*isTypeOnly*/ false,
factory.createNamedExports([exportSpecifier]),
/*moduleSpecifier*/ undefined
));
}
}
}
6 changes: 5 additions & 1 deletion src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,11 @@ namespace ts.textChanges {
}

public insertExportModifier(sourceFile: SourceFile, node: DeclarationStatement | VariableStatement): void {
this.insertText(sourceFile, node.getStart(sourceFile), "export ");
this.insertExportModifierAt(sourceFile, node.getStart(sourceFile));
}

public insertExportModifierAt(sourceFile: SourceFile, position: number): void {
Copy link
Member

Choose a reason for hiding this comment

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

I just don't quite know if you need this, but I don't understand the start + 1 thing yet.

Copy link
Author

Choose a reason for hiding this comment

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

I want to add an export identifier in front of non-exported type, you can see the codeFixImportNonExportedMember1.ts, an export added before function bar(), the start there is the position of line break, so need to +1 for start.

this.insertText(sourceFile, position, "export ");
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"codefixes/fixPropertyAssignment.ts",
"codefixes/fixExtendsInterfaceBecomesImplements.ts",
"codefixes/fixForgottenThisPropertyAccess.ts",
"codefixes/fixImportNonExportedMember.ts",
"codefixes/fixInvalidJsxCharacters.ts",
"codefixes/fixUnusedIdentifier.ts",
"codefixes/fixUnreachableCode.ts",
Expand Down
29 changes: 29 additions & 0 deletions tests/cases/fourslash/codeFixImportNonExportedMember1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/// <reference path='fourslash.ts' />
// @Filename: /a.ts
////declare function zoo(): any;
////export { zoo };

// @Filename: /b.ts
////declare function foo(): any;
////function bar(): any;
////export { foo };

// @Filename: /c.ts
////import { zoo } from "./a";
////import { bar } from "./b";

goTo.file("/c.ts");
verify.codeFixAvailable([
{ description: `Export 'bar' from module './b'` },
{ description: `Remove import from './a'` },
{ description: `Remove import from './b'` },
]);
verify.codeFix({
index: 0,
description: `Export 'bar' from module './b'`,
newFileContent: {
'/b.ts': `declare function foo(): any;
export function bar(): any;
export { foo };`
}
});
45 changes: 45 additions & 0 deletions tests/cases/fourslash/codeFixImportNonExportedMember2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/// <reference path='fourslash.ts' />

// @Filename: /a.ts
////let a = 1, b = 2, c = 3;
////export function whatever() {
////}

// @Filename: /b.ts
////let d = 4;
////export function whatever2() {
////}

// @Filename: /c.ts
////import { a } from "./a"
////import { d } from "./b"

goTo.file("/c.ts");
verify.codeFixAvailable([
{ description: `Export 'a' from module './a'` },
{ description: `Export 'd' from module './b'` },
{ description: `Remove import from './a'` },
{ description: `Remove import from './b'` },
]);
verify.codeFix({
index: 0,
description: `Export 'a' from module './a'`,
newFileContent: {
'/a.ts': `let a = 1, b = 2, c = 3;
export function whatever() {
}

export { a };
`
}
});

verify.codeFix({
index: 1,
description: `Export 'd' from module './b'`,
newFileContent: {
'/b.ts': `export let d = 4;
export function whatever2() {
}`
}
});
27 changes: 27 additions & 0 deletions tests/cases/fourslash/codeFixImportNonExportedMember3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/// <reference path='fourslash.ts' />
// @Filename: /a.ts
////let a = 1, b = 2, c = 3;
////let d = 4;
////export function whatever() {
////}
////export { d }

// @Filename: /b.ts
////import { a, d } from "./a"

goTo.file("/b.ts");
verify.codeFixAvailable([
{ description: `Export 'a' from module './a'` },
{ description: `Remove import from './a'` },
]);
verify.codeFix({
index: 0,
description: `Export 'a' from module './a'`,
newFileContent: {
'/a.ts': `let a = 1, b = 2, c = 3;
let d = 4;
export function whatever() {
}
export { a, d };`
}
});
9 changes: 9 additions & 0 deletions tests/cases/fourslash/codeFixImportNonExportedMember4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/// <reference path='fourslash.ts' />
// @Filename: /node_modules/foo/index.d.ts
////let a = 0
////module.exports = 0;

// @Filename: /a.ts
////import { a } from "foo";

verify.not.codeFixAvailable();
22 changes: 22 additions & 0 deletions tests/cases/fourslash/codeFixImportNonExportedMember_all.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/// <reference path='fourslash.ts' />

// @Filename: /a.ts
////declare function foo(): any;
////declare function bar(): any;
////declare function zoo(): any;
////export { zoo }

// @Filename: /b.ts
////import { foo, bar } from "./a";

goTo.file("/b.ts");
verify.codeFixAll({
fixId: "importNonExportedMember",
fixAllDescription: ts.Diagnostics.Add_all_missing_exports.message,
newFileContent: {
'/a.ts': `export declare function foo(): any;
export declare function bar(): any;
declare function zoo(): any;
export { zoo }`
}
});