-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: oss-hackathon-37440
Are you sure you want to change the base?
Changes from all commits
99c6de4
ffa9cf2
f3ec897
c68ae3b
5cc5f52
04f1066
a71c68f
700141e
2225781
5ff147b
6dba610
ec6324a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
/* @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.Export_all_missing_members_from_modules | ||
), | ||
]; | ||
}, | ||
|
||
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 sortSpecifiers( | ||
specifiers: ExportSpecifier[] | ||
): readonly ExportSpecifier[] { | ||
return stableSort(specifiers, (s1, s2) => | ||
compareStringsCaseInsensitive( | ||
(s1.propertyName || s1.name).text, | ||
(s2.propertyName || s2.name).text | ||
) | ||
); | ||
} | ||
|
||
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, class, or variable declaration which can have `export` prepended | ||
if ( | ||
localSymbol.valueDeclaration !== undefined && | ||
(isDeclarationStatement(localSymbol.valueDeclaration) || | ||
isVariableStatement(localSymbol.valueDeclaration)) | ||
) { | ||
const node = localSymbol.valueDeclaration; | ||
|
||
return changes.insertExportModifier(sourceFile, node); | ||
} | ||
else if ( | ||
localSymbol.valueDeclaration && | ||
isVariableDeclarationListWith1Element( | ||
localSymbol.valueDeclaration.parent | ||
) | ||
) { | ||
const node = localSymbol.valueDeclaration.parent; | ||
|
||
return changes.insertExportModifier(sourceFile, node); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need special handling for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. class is captured/handled with |
||
|
||
// In all other cases the node should be exported via `export {a}` | ||
// Search for an export statement we can use | ||
for (const namedExportDeclaration of sourceFile.statements) { | ||
if ( | ||
isExportDeclaration(namedExportDeclaration) && | ||
namedExportDeclaration.exportClause && | ||
isNamedExports(namedExportDeclaration.exportClause) && | ||
!namedExportDeclaration.isTypeOnly && // don't use `export type {...}` | ||
namedExportDeclaration.moduleSpecifier === undefined // don't use `export {...} from` | ||
) { | ||
return changes.replaceNode( | ||
sourceFile, | ||
namedExportDeclaration, | ||
factory.updateExportDeclaration( | ||
namedExportDeclaration, | ||
/*modifiers*/ undefined, | ||
/*isTypeOnly*/ false, | ||
/*exportClause*/ factory.updateNamedExports( | ||
namedExportDeclaration.exportClause, | ||
/*elements*/ sortSpecifiers( | ||
namedExportDeclaration.exportClause.elements.concat( | ||
factory.createExportSpecifier( | ||
/*isTypeOnly*/ false, | ||
/*propertyName*/ undefined, | ||
node | ||
) | ||
) | ||
) | ||
), | ||
/*moduleSpecifier*/ undefined, | ||
/*assertClause*/ undefined | ||
) | ||
); | ||
} | ||
} | ||
|
||
// If we won't find an existing `export` statement we can use, create one | ||
return changes.insertNodeAtEndOfScope( | ||
sourceFile, | ||
sourceFile, | ||
factory.createExportDeclaration( | ||
/*modifiers*/ undefined, | ||
/*isTypeOnly*/ false, | ||
/*exportClause*/ factory.createNamedExports([ | ||
factory.createExportSpecifier( | ||
/*isTypeOnly*/ false, | ||
/*propertyName*/ undefined, | ||
node | ||
), | ||
]), | ||
/*moduleSpecifier*/ undefined | ||
) | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
); | ||||||||||||
|
||||||||||||
this.insertText( | ||||||||||||
sourceFile, | ||||||||||||
node.getStart(sourceFile, /*includeJsDocComment*/ false), | ||||||||||||
"export " | ||||||||||||
); | ||||||||||||
} | ||||||||||||
|
||||||||||||
public insertImportSpecifierAtIndex(sourceFile: SourceFile, importSpecifier: ImportSpecifier, namedImports: NamedImports, index: number) { | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/// <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.Export_all_missing_members_from_modules.message, | ||
newFileContent: { | ||
"/a.ts": `export declare function foo(): any; | ||
export declare function bar(): any; | ||
declare function zoo(): any; | ||
export { zoo }`, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/// <reference path='fourslash.ts' /> | ||
// @Filename: /a.ts | ||
////class a{}, class b{}; | ||
////export let c = 3; | ||
|
||
// @Filename: /b.ts | ||
////import { a, b, c } from "./a" | ||
|
||
goTo.file("/b.ts"); | ||
verify.codeFixAvailable([ | ||
{ description: `Export 'a' from module './a'` }, | ||
{ description: `Export 'b' from module './a'` }, | ||
{ description: `Remove import from './a'` }, | ||
]); | ||
|
||
// Can export class in list | ||
verify.codeFix({ | ||
index: 1, | ||
description: `Export 'b' from module './a'`, | ||
newFileContent: { | ||
"/a.ts": `class a{}, export class b{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is producing invalid syntax. I think we need to fallback to |
||
export let c = 3;`, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/// <reference path='fourslash.ts' /> | ||
// @Filename: /a.ts | ||
////class a{}; | ||
////export let b = 2; | ||
|
||
// @Filename: /b.ts | ||
////import { a, b } from "./a" | ||
|
||
goTo.file("/b.ts"); | ||
verify.codeFixAvailable([ | ||
{ description: `Export 'a' from module './a'` }, | ||
{ description: `Remove import from './a'` }, | ||
]); | ||
// Can export a class | ||
verify.codeFix({ | ||
index: 0, | ||
description: `Export 'a' from module './a'`, | ||
newFileContent: { | ||
"/a.ts": `export class a{}; | ||
export let b = 2;`, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/// <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"); | ||
// Recognises that importing from a file with a `declare` is ok if its exported | ||
verify.codeFixAvailable([ | ||
{ description: `Export 'bar' from module './b'` }, | ||
{ description: `Remove import from './a'` }, | ||
{ description: `Remove import from './b'` }, | ||
]); | ||
// Exports a function with a `declare` correctly | ||
verify.codeFix({ | ||
index: 0, | ||
description: `Export 'bar' from module './b'`, | ||
newFileContent: { | ||
"/b.ts": `declare function foo(): any; | ||
export function bar(): any; | ||
export { foo };`, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/// <reference path='fourslash.ts' /> | ||
// @Filename: /a.ts | ||
/////** | ||
//// * baz | ||
//// */ | ||
////class a{}; | ||
////export let b = 3; | ||
|
||
// @Filename: /b.ts | ||
////import { a, b } from "./a" | ||
|
||
goTo.file("/b.ts"); | ||
verify.codeFixAvailable([ | ||
{ description: `Export 'a' from module './a'` }, | ||
{ description: `Remove import from './a'` }, | ||
]); | ||
|
||
// Doesn't clobber jsdoc on classes | ||
verify.codeFix({ | ||
index: 0, | ||
description: `Export 'a' from module './a'`, | ||
newFileContent: { | ||
"/a.ts": `/** | ||
* baz | ||
*/ | ||
export class a{}; | ||
export let b = 3;`, | ||
}, | ||
}); |
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.
minor, seems to match the other ones above this.
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.
yup nice, 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.
hmm, the
Remove import from './b'
uses single quotes, what do you think?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.
it looks like there are only 4 instances using double quotes, that seems like a mistake