Skip to content

Commit bc52ff6

Browse files
andrewbranchandrewbranch
authored
Make React import fix not block component import fix (#50307)
* Stop React import fix from blocking component import fixes * Add additional promote-type-only test
1 parent fd3c46b commit bc52ff6

File tree

4 files changed

+97
-42
lines changed

4 files changed

+97
-42
lines changed

‎src/compiler/core.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1118,6 +1118,13 @@ namespace ts {
11181118
: undefined;
11191119
}
11201120

1121+
/**
1122+
* Returns the only element of an array if it contains only one element; throws otherwise.
1123+
*/
1124+
export function single<T>(array: readonly T[]): T {
1125+
return Debug.checkDefined(singleOrUndefined(array));
1126+
}
1127+
11211128
/**
11221129
* Returns the only element of an array if it contains only one element; otherwise, returns the
11231130
* array.

‎src/services/codefixes/importFixes.ts

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,10 @@ namespace ts.codefix {
1818
errorCodes,
1919
getCodeActions(context) {
2020
const { errorCode, preferences, sourceFile, span, program } = context;
21-
const info = getFixesInfo(context, errorCode, span.start, /*useAutoImportProvider*/ true);
21+
const info = getFixInfos(context, errorCode, span.start, /*useAutoImportProvider*/ true);
2222
if (!info) return undefined;
23-
const { fixes, symbolName, errorIdentifierText } = info;
2423
const quotePreference = getQuotePreference(sourceFile, preferences);
25-
return fixes.map(fix => codeActionForFix(
24+
return info.map(({ fix, symbolName, errorIdentifierText }) => codeActionForFix(
2625
context,
2726
sourceFile,
2827
symbolName,
@@ -74,9 +73,9 @@ namespace ts.codefix {
7473
return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes, hasFixes };
7574

7675
function addImportFromDiagnostic(diagnostic: DiagnosticWithLocation, context: CodeFixContextBase) {
77-
const info = getFixesInfo(context, diagnostic.code, diagnostic.start, useAutoImportProvider);
78-
if (!info || !info.fixes.length) return;
79-
addImport(info);
76+
const info = getFixInfos(context, diagnostic.code, diagnostic.start, useAutoImportProvider);
77+
if (!info || !info.length) return;
78+
addImport(first(info));
8079
}
8180

8281
function addImportFromExportedSymbol(exportedSymbol: Symbol, isValidTypeOnlyUseSite?: boolean) {
@@ -88,13 +87,12 @@ namespace ts.codefix {
8887
const useRequire = shouldUseRequire(sourceFile, program);
8988
const fix = getImportFixForSymbol(sourceFile, exportInfo, moduleSymbol, program, /*useNamespaceInfo*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
9089
if (fix) {
91-
addImport({ fixes: [fix], symbolName, errorIdentifierText: undefined });
90+
addImport({ fix, symbolName, errorIdentifierText: undefined });
9291
}
9392
}
9493

95-
function addImport(info: FixesInfo) {
96-
const { fixes, symbolName } = info;
97-
const fix = first(fixes);
94+
function addImport(info: FixInfo) {
95+
const { fix, symbolName } = info;
9896
switch (fix.kind) {
9997
case ImportFixKind.UseNamespace:
10098
addToNamespace.push(fix);
@@ -369,7 +367,7 @@ namespace ts.codefix {
369367

370368
export function getPromoteTypeOnlyCompletionAction(sourceFile: SourceFile, symbolToken: Identifier, program: Program, host: LanguageServiceHost, formatContext: formatting.FormatContext, preferences: UserPreferences) {
371369
const compilerOptions = program.getCompilerOptions();
372-
const symbolName = getSymbolName(sourceFile, program.getTypeChecker(), symbolToken, compilerOptions);
370+
const symbolName = single(getSymbolNamesToImport(sourceFile, program.getTypeChecker(), symbolToken, compilerOptions));
373371
const fix = getTypeOnlyPromotionFix(sourceFile, symbolToken, symbolName, program);
374372
const includeSymbolNameInDescription = symbolName !== symbolToken.text;
375373
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, QuotePreference.Double, compilerOptions));
@@ -741,8 +739,13 @@ namespace ts.codefix {
741739
}
742740
}
743741

744-
interface FixesInfo { readonly fixes: readonly ImportFix[], readonly symbolName: string, readonly errorIdentifierText: string | undefined }
745-
function getFixesInfo(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): FixesInfo | undefined {
742+
interface FixInfo {
743+
readonly fix: ImportFix;
744+
readonly symbolName: string;
745+
readonly errorIdentifierText: string | undefined;
746+
readonly isJsxNamespaceFix?: boolean;
747+
}
748+
function getFixInfos(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): readonly FixInfo[] | undefined {
746749
const symbolToken = getTokenAtPosition(context.sourceFile, pos);
747750
let info;
748751
if (errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code) {
@@ -752,21 +755,24 @@ namespace ts.codefix {
752755
return undefined;
753756
}
754757
else if (errorCode === Diagnostics._0_cannot_be_used_as_a_value_because_it_was_imported_using_import_type.code) {
755-
const symbolName = getSymbolName(context.sourceFile, context.program.getTypeChecker(), symbolToken, context.program.getCompilerOptions());
758+
const symbolName = single(getSymbolNamesToImport(context.sourceFile, context.program.getTypeChecker(), symbolToken, context.program.getCompilerOptions()));
756759
const fix = getTypeOnlyPromotionFix(context.sourceFile, symbolToken, symbolName, context.program);
757-
return fix && { fixes: [fix], symbolName, errorIdentifierText: symbolToken.text };
760+
return fix && [{ fix, symbolName, errorIdentifierText: symbolToken.text }];
758761
}
759762
else {
760763
info = getFixesInfoForNonUMDImport(context, symbolToken, useAutoImportProvider);
761764
}
762765

763766
const packageJsonImportFilter = createPackageJsonImportFilter(context.sourceFile, context.preferences, context.host);
764-
return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, packageJsonImportFilter, context.host) };
767+
return info && sortFixInfo(info, context.sourceFile, context.program, packageJsonImportFilter, context.host);
765768
}
766769

767-
function sortFixes(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): readonly ImportFixWithModuleSpecifier[] {
770+
function sortFixInfo(fixes: readonly (FixInfo & { fix: ImportFixWithModuleSpecifier })[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): readonly (FixInfo & { fix: ImportFixWithModuleSpecifier })[] {
768771
const _toPath = (fileName: string) => toPath(fileName, host.getCurrentDirectory(), hostGetCanonicalFileName(host));
769-
return sort(fixes, (a, b) => compareValues(a.kind, b.kind) || compareModuleSpecifiers(a, b, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier, _toPath));
772+
return sort(fixes, (a, b) =>
773+
compareBooleans(!!a.isJsxNamespaceFix, !!b.isJsxNamespaceFix) ||
774+
compareValues(a.fix.kind, b.fix.kind) ||
775+
compareModuleSpecifiers(a.fix, b.fix, sourceFile, program, packageJsonImportFilter.allowsImportingSpecifier, _toPath));
770776
}
771777

772778
function getBestFix(fixes: readonly ImportFixWithModuleSpecifier[], sourceFile: SourceFile, program: Program, packageJsonImportFilter: PackageJsonImportFilter, host: LanguageServiceHost): ImportFixWithModuleSpecifier | undefined {
@@ -836,7 +842,7 @@ namespace ts.codefix {
836842
return Comparison.EqualTo;
837843
}
838844

839-
function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): FixesInfo & { fixes: readonly ImportFixWithModuleSpecifier[] } | undefined {
845+
function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): (FixInfo & { fix: ImportFixWithModuleSpecifier })[] | undefined {
840846
const checker = program.getTypeChecker();
841847
const umdSymbol = getUmdSymbol(token, checker);
842848
if (!umdSymbol) return undefined;
@@ -846,7 +852,7 @@ namespace ts.codefix {
846852
const useRequire = shouldUseRequire(sourceFile, program);
847853
const position = isIdentifier(token) ? token.getStart(sourceFile) : undefined;
848854
const fixes = getImportFixes(exportInfo, position ? { position, symbolName } : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences).fixes;
849-
return { fixes, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text };
855+
return fixes.map(fix => ({ fix, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text }));
850856
}
851857
function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined {
852858
// try the identifier to see if it is the umd symbol
@@ -906,20 +912,21 @@ namespace ts.codefix {
906912
}
907913
}
908914

909-
function getFixesInfoForNonUMDImport({ sourceFile, program, cancellationToken, host, preferences }: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean): FixesInfo & { fixes: readonly ImportFixWithModuleSpecifier[] } | undefined {
915+
function getFixesInfoForNonUMDImport({ sourceFile, program, cancellationToken, host, preferences }: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean): readonly (FixInfo & { fix: ImportFixWithModuleSpecifier })[] | undefined {
910916
const checker = program.getTypeChecker();
911917
const compilerOptions = program.getCompilerOptions();
912-
const symbolName = getSymbolName(sourceFile, checker, symbolToken, compilerOptions);
913-
// "default" is a keyword and not a legal identifier for the import, but appears as an identifier.
914-
if (symbolName === InternalSymbolName.Default) {
915-
return undefined;
916-
}
917-
const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken);
918-
const useRequire = shouldUseRequire(sourceFile, program);
919-
const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
920-
const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
921-
getImportFixes(exportInfos, { symbolName, position: symbolToken.getStart(sourceFile) }, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes));
922-
return { fixes, symbolName, errorIdentifierText: symbolToken.text };
918+
return flatMap(getSymbolNamesToImport(sourceFile, checker, symbolToken, compilerOptions), symbolName => {
919+
// "default" is a keyword and not a legal identifier for the import, but appears as an identifier.
920+
if (symbolName === InternalSymbolName.Default) {
921+
return undefined;
922+
}
923+
const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken);
924+
const useRequire = shouldUseRequire(sourceFile, program);
925+
const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
926+
const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
927+
getImportFixes(exportInfos, { symbolName, position: symbolToken.getStart(sourceFile) }, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes));
928+
return fixes.map(fix => ({ fix, symbolName, errorIdentifierText: symbolToken.text, isJsxNamespaceFix: symbolName !== symbolToken.text }));
929+
});
923930
}
924931

925932
function getTypeOnlyPromotionFix(sourceFile: SourceFile, symbolToken: Identifier, symbolName: string, program: Program): FixPromoteTypeOnlyImport | undefined {
@@ -933,15 +940,16 @@ namespace ts.codefix {
933940
return { kind: ImportFixKind.PromoteTypeOnly, typeOnlyAliasDeclaration };
934941
}
935942

936-
function getSymbolName(sourceFile: SourceFile, checker: TypeChecker, symbolToken: Identifier, compilerOptions: CompilerOptions): string {
943+
function getSymbolNamesToImport(sourceFile: SourceFile, checker: TypeChecker, symbolToken: Identifier, compilerOptions: CompilerOptions): string[] {
937944
const parent = symbolToken.parent;
938945
if ((isJsxOpeningLikeElement(parent) || isJsxClosingElement(parent)) && parent.tagName === symbolToken && jsxModeNeedsExplicitImport(compilerOptions.jsx)) {
939946
const jsxNamespace = checker.getJsxNamespace(sourceFile);
940947
if (needsJsxNamespaceFix(jsxNamespace, symbolToken, checker)) {
941-
return jsxNamespace;
948+
const needsComponentNameFix = !isIntrinsicJsxName(symbolToken.text) && !checker.resolveName(symbolToken.text, symbolToken, SymbolFlags.Value, /*excludeGlobals*/ false);
949+
return needsComponentNameFix ? [symbolToken.text, jsxNamespace] : [jsxNamespace];
942950
}
943951
}
944-
return symbolToken.text;
952+
return [symbolToken.text];
945953
}
946954

947955
function needsJsxNamespaceFix(jsxNamespace: string, symbolToken: Identifier, checker: TypeChecker) {

‎tests/cases/fourslash/importNameCodeFix_importType9.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/// <reference path="fourslash.ts" />
2+
// @module: es2015
3+
// @esModuleInterop: true
4+
// @jsx: react
5+
6+
// @Filename: /types.d.ts
7+
//// declare module "react" { var React: any; export = React; export as namespace React; }
8+
9+
// @Filename: /component.tsx
10+
//// export function Component() { return <div />; }
11+
12+
// @Filename: /a.tsx
13+
//// import type React from "react";
14+
//// import type { Component } from "./component";
15+
//// (<Component/**/ />)
16+
17+
goTo.marker("");
18+
19+
// It would be preferable for these fixes to be provided simultaneously, like the add-new-import fixes are,
20+
// but this is such a weird edge case that I don't know that it's worth it - the test mainly ensures that
21+
// both fixes are eventually offered without crashing and that they do what they say they're going to do.
22+
23+
verify.codeFix({
24+
index: 0,
25+
description: [ts.Diagnostics.Remove_type_from_import_declaration_from_0.message, "react"],
26+
applyChanges: true,
27+
newFileContent: `import React from "react";
28+
import type { Component } from "./component";
29+
(<Component />)`
30+
});
31+
32+
verify.codeFix({
33+
index: 0,
34+
description: [ts.Diagnostics.Remove_type_from_import_declaration_from_0.message, "./component"],
35+
applyChanges: true,
36+
newFileContent: `import React from "react";
37+
import { Component } from "./component";
38+
(<Component />)`
39+
});

‎tests/cases/fourslash/importNameCodeFix_jsx6.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,22 +20,23 @@
2020
////<[|Text|]></Text>;
2121

2222
goTo.file("/a.tsx");
23+
2324
verify.codeFix({
2425
index: 0,
25-
description: [ts.Diagnostics.Import_0_from_1.message, "React", "react"],
26-
applyChanges: true,
26+
description: [ts.Diagnostics.Add_import_from_0.message, "react-native"],
27+
applyChanges: false,
2728
newFileContent:
28-
`import React from "react";
29+
`import { Text } from "react-native";
2930
3031
<Text></Text>;`
3132
});
3233

3334
verify.codeFix({
34-
index: 0,
35-
description: [ts.Diagnostics.Add_import_from_0.message, "react-native"],
36-
newFileContent:
35+
index: 1,
36+
description: [ts.Diagnostics.Import_0_from_1.message, "React", "react"],
37+
applyChanges: false,
38+
newFileContent:
3739
`import React from "react";
38-
import { Text } from "react-native";
3940
4041
<Text></Text>;`
4142
});

0 commit comments

Comments
 (0)