Skip to content

Make React import fix not block component import fix #50307

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

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Make React import fix not block component import fix #50307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfa5a7f
Stop React import fix from blocking component import fixes
andrewbranch Aug 15, 2022
3ae2899
Add additional promote-type-only test
andrewbranch Aug 15, 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
7 changes: 7 additions & 0 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,13 @@ namespace ts {
: undefined;
}

/**
* Returns the only element of an array if it contains only one element; throws otherwise.
*/
export function single<T>(array: readonly T[]): T {
return Debug.checkDefined(singleOrUndefined(array));
}

/**
* Returns the only element of an array if it contains only one element; otherwise, returns the
* array.
Expand Down
78 changes: 43 additions & 35 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@ namespace ts.codefix {
errorCodes,
getCodeActions(context) {
const { errorCode, preferences, sourceFile, span, program } = context;
const info = getFixesInfo(context, errorCode, span.start, /*useAutoImportProvider*/ true);
const info = getFixInfos(context, errorCode, span.start, /*useAutoImportProvider*/ true);
if (!info) return undefined;
const { fixes, symbolName, errorIdentifierText } = info;
const quotePreference = getQuotePreference(sourceFile, preferences);
return fixes.map(fix => codeActionForFix(
return info.map(({ fix, symbolName, errorIdentifierText }) => codeActionForFix(
context,
sourceFile,
symbolName,
Expand Down Expand Up @@ -74,9 +73,9 @@ namespace ts.codefix {
return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes, hasFixes };

function addImportFromDiagnostic(diagnostic: DiagnosticWithLocation, context: CodeFixContextBase) {
const info = getFixesInfo(context, diagnostic.code, diagnostic.start, useAutoImportProvider);
if (!info || !info.fixes.length) return;
addImport(info);
const info = getFixInfos(context, diagnostic.code, diagnostic.start, useAutoImportProvider);
if (!info || !info.length) return;
addImport(first(info));
}

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

function addImport(info: FixesInfo) {
const { fixes, symbolName } = info;
const fix = first(fixes);
function addImport(info: FixInfo) {
const { fix, symbolName } = info;
switch (fix.kind) {
case ImportFixKind.UseNamespace:
addToNamespace.push(fix);
Expand Down Expand Up @@ -369,7 +367,7 @@ namespace ts.codefix {

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

interface FixesInfo { readonly fixes: readonly ImportFix[], readonly symbolName: string, readonly errorIdentifierText: string | undefined }
function getFixesInfo(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): FixesInfo | undefined {
interface FixInfo {
readonly fix: ImportFix;
readonly symbolName: string;
readonly errorIdentifierText: string | undefined;
readonly isJsxNamespaceFix?: boolean;
}
function getFixInfos(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): readonly FixInfo[] | undefined {
const symbolToken = getTokenAtPosition(context.sourceFile, pos);
let info;
if (errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code) {
Expand All @@ -752,21 +755,24 @@ namespace ts.codefix {
return undefined;
}
else if (errorCode === Diagnostics._0_cannot_be_used_as_a_value_because_it_was_imported_using_import_type.code) {
const symbolName = getSymbolName(context.sourceFile, context.program.getTypeChecker(), symbolToken, context.program.getCompilerOptions());
const symbolName = single(getSymbolNamesToImport(context.sourceFile, context.program.getTypeChecker(), symbolToken, context.program.getCompilerOptions()));
const fix = getTypeOnlyPromotionFix(context.sourceFile, symbolToken, symbolName, context.program);
return fix && { fixes: [fix], symbolName, errorIdentifierText: symbolToken.text };
return fix && [{ fix, symbolName, errorIdentifierText: symbolToken.text }];
}
else {
info = getFixesInfoForNonUMDImport(context, symbolToken, useAutoImportProvider);
}

const packageJsonImportFilter = createPackageJsonImportFilter(context.sourceFile, context.preferences, context.host);
return info && { ...info, fixes: sortFixes(info.fixes, context.sourceFile, context.program, packageJsonImportFilter, context.host) };
return info && sortFixInfo(info, context.sourceFile, context.program, packageJsonImportFilter, context.host);
}

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

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

function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): FixesInfo & { fixes: readonly ImportFixWithModuleSpecifier[] } | undefined {
function getFixesInfoForUMDImport({ sourceFile, program, host, preferences }: CodeFixContextBase, token: Node): (FixInfo & { fix: ImportFixWithModuleSpecifier })[] | undefined {
const checker = program.getTypeChecker();
const umdSymbol = getUmdSymbol(token, checker);
if (!umdSymbol) return undefined;
Expand All @@ -846,7 +852,7 @@ namespace ts.codefix {
const useRequire = shouldUseRequire(sourceFile, program);
const position = isIdentifier(token) ? token.getStart(sourceFile) : undefined;
const fixes = getImportFixes(exportInfo, position ? { position, symbolName } : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences).fixes;
return { fixes, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text };
return fixes.map(fix => ({ fix, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text }));
}
function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined {
// try the identifier to see if it is the umd symbol
Expand Down Expand Up @@ -906,20 +912,21 @@ namespace ts.codefix {
}
}

function getFixesInfoForNonUMDImport({ sourceFile, program, cancellationToken, host, preferences }: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean): FixesInfo & { fixes: readonly ImportFixWithModuleSpecifier[] } | undefined {
function getFixesInfoForNonUMDImport({ sourceFile, program, cancellationToken, host, preferences }: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean): readonly (FixInfo & { fix: ImportFixWithModuleSpecifier })[] | undefined {
const checker = program.getTypeChecker();
const compilerOptions = program.getCompilerOptions();
const symbolName = getSymbolName(sourceFile, checker, symbolToken, compilerOptions);
// "default" is a keyword and not a legal identifier for the import, but appears as an identifier.
if (symbolName === InternalSymbolName.Default) {
return undefined;
}
const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken);
const useRequire = shouldUseRequire(sourceFile, program);
const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
getImportFixes(exportInfos, { symbolName, position: symbolToken.getStart(sourceFile) }, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes));
return { fixes, symbolName, errorIdentifierText: symbolToken.text };
return flatMap(getSymbolNamesToImport(sourceFile, checker, symbolToken, compilerOptions), symbolName => {
// "default" is a keyword and not a legal identifier for the import, but appears as an identifier.
if (symbolName === InternalSymbolName.Default) {
return undefined;
}
const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(symbolToken);
const useRequire = shouldUseRequire(sourceFile, program);
const exportInfo = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
const fixes = arrayFrom(flatMapIterator(exportInfo.entries(), ([_, exportInfos]) =>
getImportFixes(exportInfos, { symbolName, position: symbolToken.getStart(sourceFile) }, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes));
return fixes.map(fix => ({ fix, symbolName, errorIdentifierText: symbolToken.text, isJsxNamespaceFix: symbolName !== symbolToken.text }));
});
}

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

function getSymbolName(sourceFile: SourceFile, checker: TypeChecker, symbolToken: Identifier, compilerOptions: CompilerOptions): string {
function getSymbolNamesToImport(sourceFile: SourceFile, checker: TypeChecker, symbolToken: Identifier, compilerOptions: CompilerOptions): string[] {
const parent = symbolToken.parent;
if ((isJsxOpeningLikeElement(parent) || isJsxClosingElement(parent)) && parent.tagName === symbolToken && jsxModeNeedsExplicitImport(compilerOptions.jsx)) {
const jsxNamespace = checker.getJsxNamespace(sourceFile);
if (needsJsxNamespaceFix(jsxNamespace, symbolToken, checker)) {
return jsxNamespace;
const needsComponentNameFix = !isIntrinsicJsxName(symbolToken.text) && !checker.resolveName(symbolToken.text, symbolToken, SymbolFlags.Value, /*excludeGlobals*/ false);
return needsComponentNameFix ? [symbolToken.text, jsxNamespace] : [jsxNamespace];
}
}
return symbolToken.text;
return [symbolToken.text];
}

function needsJsxNamespaceFix(jsxNamespace: string, symbolToken: Identifier, checker: TypeChecker) {
Expand Down
39 changes: 39 additions & 0 deletions tests/cases/fourslash/importNameCodeFix_importType9.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/// <reference path="fourslash.ts" />
// @module: es2015
// @esModuleInterop: true
// @jsx: react

// @Filename: /types.d.ts
//// declare module "react" { var React: any; export = React; export as namespace React; }

// @Filename: /component.tsx
//// export function Component() { return <div />; }

// @Filename: /a.tsx
//// import type React from "react";
//// import type { Component } from "./component";
//// (<Component/**/ />)

goTo.marker("");

// It would be preferable for these fixes to be provided simultaneously, like the add-new-import fixes are,
// but this is such a weird edge case that I don't know that it's worth it - the test mainly ensures that
// both fixes are eventually offered without crashing and that they do what they say they're going to do.

verify.codeFix({
index: 0,
description: [ts.Diagnostics.Remove_type_from_import_declaration_from_0.message, "react"],
applyChanges: true,
newFileContent: `import React from "react";
import type { Component } from "./component";
(<Component />)`
});

verify.codeFix({
index: 0,
description: [ts.Diagnostics.Remove_type_from_import_declaration_from_0.message, "./component"],
applyChanges: true,
newFileContent: `import React from "react";
import { Component } from "./component";
(<Component />)`
});
15 changes: 8 additions & 7 deletions tests/cases/fourslash/importNameCodeFix_jsx6.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,23 @@
////<[|Text|]></Text>;

goTo.file("/a.tsx");

verify.codeFix({
index: 0,
description: [ts.Diagnostics.Import_0_from_1.message, "React", "react"],
applyChanges: true,
description: [ts.Diagnostics.Add_import_from_0.message, "react-native"],
applyChanges: false,
newFileContent:
`import React from "react";
`import { Text } from "react-native";

<Text></Text>;`
});

verify.codeFix({
index: 0,
description: [ts.Diagnostics.Add_import_from_0.message, "react-native"],
newFileContent:
index: 1,
description: [ts.Diagnostics.Import_0_from_1.message, "React", "react"],
applyChanges: false,
newFileContent:
`import React from "react";
import { Text } from "react-native";

<Text></Text>;`
});