Skip to content
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#Provide a quick-fix for non-exported imports #72

Open
wants to merge 12 commits into
base: oss-hackathon-37440
Choose a base branch
from
9 changes: 8 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -6598,7 +6598,14 @@
"category": "Message",
"code": 90058
},

"Export '{0}' from module '{1}'": {

Choose a reason for hiding this comment

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

Suggested change
"Export '{0}' from module '{1}'": {
"Export \"{0}\" from module \"{1}\"": {

minor, seems to match the other ones above this.

Copy link
Author

Choose a reason for hiding this comment

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

yup nice, done

Copy link
Author

Choose a reason for hiding this comment

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

hmm, the Remove import from './b' uses single quotes, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

it looks like there are only 4 instances using double quotes, that seems like a mistake
image
image

"category": "Message",
"code": 90059
},
"Add all missing exports": {

Choose a reason for hiding this comment

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

I realise this message is simialr to the "Add all imports" one, but I don't think it really expresses the change that will happen. Maybe "Export all missing members in modules"

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, I changed to Export all missing members from modules

"category": "Message",
"code": 90060
},
"Convert function to an ES2015 class": {
"category": "Message",
"code": 95001
Expand Down
229 changes: 229 additions & 0 deletions src/services/codefixes/fixImportNonExportedMember.ts
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)

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
Author

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);
}

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
Author

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)

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
Author

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
)

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
Author

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
),
]),

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
)
);
}
}
}
16 changes: 14 additions & 2 deletions src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -780,8 +780,20 @@ namespace ts.textChanges {
}
}

public insertExportModifier(sourceFile: SourceFile, node: DeclarationStatement | VariableStatement): void {
this.insertText(sourceFile, node.getStart(sourceFile), "export ");
public insertExportModifier(
sourceFile: SourceFile,
node: DeclarationStatement | VariableStatement | VariableDeclarationList
): void {
Debug.assert(
/*expression*/ !(isVariableDeclarationList(node) && node.declarations.length !== 1),
/*message*/ "Only allow adding export modifier to variable lists with 1 element"
Comment on lines +788 to +789

Choose a reason for hiding this comment

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

Suggested change
/*expression*/ !(isVariableDeclarationList(node) && node.declarations.length !== 1),
/*message*/ "Only allow adding export modifier to variable lists with 1 element"
!(isVariableDeclarationList(node) && node.declarations.length !== 1),
"Only allow adding export modifier to variable lists with 1 element"

);

this.insertText(
sourceFile,
node.getStart(sourceFile, /*includeJsDocComment*/ false),
"export "
);
}

public insertImportSpecifierAtIndex(sourceFile: SourceFile, importSpecifier: ImportSpecifier, namedImports: NamedImports, index: number) {
Expand Down
3 changes: 2 additions & 1 deletion src/services/tsconfig.json
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",
]
}
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;

Choose a reason for hiding this comment

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

I would like to see some tests on overloaded functions as well.

Copy link
Author

Choose a reason for hiding this comment

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

Good shout! do you think the export should be on the implemented function or in a named export? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I've given this a look and its actually a little tricky given the current implementation!

I can easily make all functions export using a named export => export {add};, but if we want to follow the convention of applying the export on the declaration where possible it'll take a bit more work! Do you think that's critical?

Choose a reason for hiding this comment

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

I think a separate export statement (e.g. export { add }) would be acceptable for function overloads.

////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 }

Choose a reason for hiding this comment

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

Would be good to add some test cases where source file has some exports that we cannot use:

export type { a };
export { b } from "./b";
export * as c from "./c";
// ...

Copy link
Author

Choose a reason for hiding this comment

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

done


// @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();
Loading