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

importFixes: Support missing "React" at a JSXOpeningElement #16066

Merged
6 commits merged into from
Jun 5, 2017
Merged
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
4 changes: 2 additions & 2 deletions scripts/tslint/nextLineRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class Rule extends Lint.Rules.AbstractRule {

function walk(ctx: Lint.WalkContext<void>, checkCatch: boolean, checkElse: boolean): void {
const { sourceFile } = ctx;
function recur(node: ts.Node): void {
ts.forEachChild(sourceFile, function recur(node) {
switch (node.kind) {
case ts.SyntaxKind.IfStatement:
checkIf(node as ts.IfStatement);
Expand All @@ -28,7 +28,7 @@ function walk(ctx: Lint.WalkContext<void>, checkCatch: boolean, checkElse: boole
break;
}
ts.forEachChild(node, recur);
}
});

function checkIf(node: ts.IfStatement): void {
const { thenStatement, elseStatement } = node;
Expand Down
12 changes: 9 additions & 3 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ namespace ts {
getSuggestionForNonexistentProperty,
getSuggestionForNonexistentSymbol,
getBaseConstraintOfType,
getJsxNamespace,
resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined {
location = getParseTreeNode(location);
return resolveName(location, name, meaning, /*nameNotFoundMessage*/ undefined, name);
},
};

const tupleTypes: GenericType[] = [];
Expand Down Expand Up @@ -847,7 +852,7 @@ namespace ts {
location: Node | undefined,
name: string,
meaning: SymbolFlags,
nameNotFoundMessage: DiagnosticMessage,
nameNotFoundMessage: DiagnosticMessage | undefined,
nameArg: string | Identifier,
suggestedNameNotFoundMessage?: DiagnosticMessage): Symbol {
return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, getSymbol, suggestedNameNotFoundMessage);
Expand Down Expand Up @@ -5841,7 +5846,8 @@ namespace ts {
}
}
return arrayFrom(props.values());
} else {
}
else {
return getPropertiesOfType(type);
}
}
Expand Down Expand Up @@ -14118,7 +14124,7 @@ namespace ts {
checkJsxPreconditions(node);
// The reactNamespace/jsxFactory's root symbol should be marked as 'used' so we don't incorrectly elide its import.
// And if there is no reactNamespace/jsxFactory's symbol in scope when targeting React emit, we should issue an error.
const reactRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
const reactRefErr = diagnostics && compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
const reactNamespace = getJsxNamespace();
const reactSym = resolveName(node.tagName, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace);
if (reactSym) {
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1483,7 +1483,8 @@ namespace ts {
}
}
return sourceFile;
} else {
}
else {
const sourceFileNoExtension = options.allowNonTsExtensions && getSourceFile(fileName);
if (sourceFileNoExtension) return sourceFileNoExtension;

Expand Down
3 changes: 3 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2602,6 +2602,9 @@ namespace ts {
* Does not include properties of primitive types.
*/
/* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[];

/* @internal */ getJsxNamespace(): string;
/* @internal */ resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined;
}

export enum NodeBuilderFlags {
Expand Down
5 changes: 4 additions & 1 deletion src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2385,7 +2385,10 @@ namespace FourSlash {
const codeFixes = this.getCodeFixActions(this.activeFile.fileName, errorCode);

if (!codeFixes || codeFixes.length === 0) {
this.raiseError("No codefixes returned.");
if (expectedTextArray.length !== 0) {
this.raiseError("No codefixes returned.");
}
return;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this change was made.

Copy link
Author

Choose a reason for hiding this comment

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

importNameCodeFixUMDGlobalReact2.ts asserts that the code fixes are currently empty. Previously this method would have raised an error "No codefixes returned" even if that was exactly what you were asserting.

}

const actualTextArray: string[] = [];
Expand Down
3 changes: 2 additions & 1 deletion src/harness/unittests/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ namespace ts.projectSystem {
}

getEvent<T extends server.ProjectServiceEvent>(eventName: T["eventName"], mayBeMore = false): T["data"] {
if (mayBeMore) assert(this.events.length !== 0); else assert.equal(this.events.length, 1);
if (mayBeMore) { assert(this.events.length !== 0); }
else { assert.equal(this.events.length, 1); }
const event = this.events.shift();
assert.equal(event.eventName, eventName);
return event.data;
Expand Down
38 changes: 27 additions & 11 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,23 @@ namespace ts.codefix {

const currentTokenMeaning = getMeaningFromLocation(token);
if (context.errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code) {
const symbol = checker.getAliasedSymbol(checker.getSymbolAtLocation(token));
return getCodeActionForImport(symbol, /*isDefault*/ false, /*isNamespaceImport*/ true);
const umdSymbol = checker.getSymbolAtLocation(token);
let symbol: ts.Symbol;
let symbolName: string;
if (umdSymbol.flags & ts.SymbolFlags.Alias) {
symbol = checker.getAliasedSymbol(umdSymbol);
symbolName = name;
}
else if (isJsxOpeningLikeElement(token.parent) && token.parent.tagName === token) {
// The error wasn't for the symbolAtLocation, it was for the JSX tag itself, which needs access to e.g. `React`.
symbol = checker.getAliasedSymbol(checker.resolveNameAtLocation(token, checker.getJsxNamespace(), SymbolFlags.Value));
symbolName = symbol.name;
}
else {
Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
}

return getCodeActionForImport(symbol, symbolName, /*isDefault*/ false, /*isNamespaceImport*/ true);
}

const candidateModules = checker.getAmbientModules();
Expand All @@ -159,15 +174,15 @@ namespace ts.codefix {
if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) {
// check if this symbol is already used
const symbolId = getUniqueSymbolId(localSymbol);
symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, /*isDefault*/ true));
symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, name, /*isDefault*/ true));
}
}

// check exports with the same name
const exportSymbolWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol);
if (exportSymbolWithIdenticalName && checkSymbolHasMeaning(exportSymbolWithIdenticalName, currentTokenMeaning)) {
const symbolId = getUniqueSymbolId(exportSymbolWithIdenticalName);
symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol));
symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, name));
}
}

Expand Down Expand Up @@ -218,7 +233,7 @@ namespace ts.codefix {
return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false;
}

function getCodeActionForImport(moduleSymbol: Symbol, isDefault?: boolean, isNamespaceImport?: boolean): ImportCodeAction[] {
function getCodeActionForImport(moduleSymbol: Symbol, symbolName: string, isDefault?: boolean, isNamespaceImport?: boolean): ImportCodeAction[] {
const existingDeclarations = getImportDeclarations(moduleSymbol);
if (existingDeclarations.length > 0) {
// With an existing import statement, there are more than one actions the user can do.
Expand Down Expand Up @@ -375,10 +390,10 @@ namespace ts.codefix {
const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport());
const changeTracker = createChangeTracker();
const importClause = isDefault
? createImportClause(createIdentifier(name), /*namedBindings*/ undefined)
? createImportClause(createIdentifier(symbolName), /*namedBindings*/ undefined)
: isNamespaceImport
? createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(name)))
: createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(name))]));
? createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(symbolName)))
: createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(symbolName))]));
const importDecl = createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, importClause, createLiteral(moduleSpecifierWithoutQuotes));
if (!lastImportDeclaration) {
changeTracker.insertNodeAt(sourceFile, sourceFile.getStart(), importDecl, { suffix: `${context.newLineCharacter}${context.newLineCharacter}` });
Expand All @@ -392,7 +407,7 @@ namespace ts.codefix {
// are there are already a new line seperating code and import statements.
return createCodeAction(
Diagnostics.Import_0_from_1,
[name, `"${moduleSpecifierWithoutQuotes}"`],
[symbolName, `"${moduleSpecifierWithoutQuotes}"`],
changeTracker.getChanges(),
"NewImport",
moduleSpecifierWithoutQuotes
Expand All @@ -412,8 +427,9 @@ namespace ts.codefix {
removeFileExtension(getRelativePath(moduleFileName, sourceDirectory));

function tryGetModuleNameFromAmbientModule(): string {
if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) {
return moduleSymbol.name;
const decl = moduleSymbol.valueDeclaration;
if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) {
return decl.name.text;
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ namespace ts.FindAllReferences {
if (entry.type === "node") {
const { node } = entry;
return { textSpan: getTextSpan(node), fileName: node.getSourceFile().fileName, ...implementationKindDisplayParts(node, checker) };
} else {
}
else {
const { textSpan, fileName } = entry;
return { textSpan, fileName, kind: ScriptElementKind.unknown, displayParts: [] };
}
Expand Down
3 changes: 2 additions & 1 deletion src/services/importTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,8 @@ namespace ts.FindAllReferences {
if (parent.kind === SyntaxKind.VariableDeclaration) {
const p = parent as ts.VariableDeclaration;
return p.parent.kind === ts.SyntaxKind.CatchClause ? undefined : p.parent.parent.kind === SyntaxKind.VariableStatement ? p.parent.parent : undefined;
} else {
}
else {
return parent;
}
}
Expand Down
28 changes: 28 additions & 0 deletions tests/cases/fourslash/importNameCodeFixUMDGlobalReact0.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/// <reference path="fourslash.ts" />

// @jsx: react

// @Filename: /node_modules/@types/react/index.d.ts
////export = React;
////export as namespace React;
////declare namespace React {
//// export class Component { render(): JSX.Element | null; }
////}
////declare global {
//// namespace JSX {
//// interface Element {}
//// }
////}

// @Filename: /a.tsx
////[|import { Component } from "react";
////export class MyMap extends Component { }
////<MyMap/>;|]

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

verify.importFixAtPosition([
`import { Component } from "react";
import * as React from "react";
export class MyMap extends Component { }
<MyMap/>;`]);
28 changes: 28 additions & 0 deletions tests/cases/fourslash/importNameCodeFixUMDGlobalReact1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/// <reference path="fourslash.ts" />

// @jsx: react

// @Filename: /node_modules/@types/react/index.d.ts
////export = React;
////export as namespace React;
////declare namespace React {
//// export class Component { render(): JSX.Element | null; }
////}
////declare global {
//// namespace JSX {
//// interface Element {}
//// }
////}

// @Filename: /a.tsx
////[|import { Component } from "react";
////export class MyMap extends Component { }
////<MyMap></MyMap>;|]

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

verify.importFixAtPosition([
`import { Component } from "react";
import * as React from "react";
export class MyMap extends Component { }
<MyMap></MyMap>;`]);
21 changes: 21 additions & 0 deletions tests/cases/fourslash/importNameCodeFixUMDGlobalReact2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path="fourslash.ts" />

// https://github.com/Microsoft/TypeScript/issues/16065

// @jsx: react
// @jsxFactory: factory

// @Filename: /factory.ts
////export function factory() { return {}; }
////declare global {
//// namespace JSX {
//// interface Element {}
//// }
////}

// @Filename: /a.tsx
////[|<div/>|]

goTo.file("/a.tsx");
verify.not
verify.importFixAtPosition([]);