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 1 commit
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
14 changes: 11 additions & 3 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ namespace ts {
getSuggestionForNonexistentProperty,
getSuggestionForNonexistentSymbol,
getBaseConstraintOfType,
getJsxNamespaceSymbol: node => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a misleading name - why not getSymbolFromJsxNamespace?

Copy link
Member

Choose a reason for hiding this comment

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

Also, why not just return the JSX namespace itself and look up the original entity in the exports for that symbol?

Copy link
Author

@ghost ghost May 25, 2017

Choose a reason for hiding this comment

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

This method returns the JSX namespace symbol, such as React. It doesn't take it as a parameter. It's useful if the JSX namespace was missing and we can import it.
The function getJsxNamespace just returns a string, so that isn't usable outside of the checker.

Copy link
Member

@DanielRosenwasser DanielRosenwasser May 25, 2017

Choose a reason for hiding this comment

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

After reading through, I think I see why I was confused. JSX is usually a global, but in theory it could be locally imported. In that case, maybe resolveJsxNamespaceAtLocation is more accurate.

node = getParseTreeNode(node);
return getJsxNamespaceSymbol(node, /*diagnostics*/ false);
},
};

const tupleTypes: GenericType[] = [];
Expand Down Expand Up @@ -14101,9 +14105,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 reactNamespace = getJsxNamespace();
const reactSym = resolveName(node.tagName, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace);
const reactSym = getJsxNamespaceSymbol(node.tagName, /*diagnostics*/ true);
if (reactSym) {
// Mark local symbol as referenced here because it might not have been marked
// if jsx emit was not react as there wont be error being emitted
Expand All @@ -14118,6 +14120,12 @@ namespace ts {
checkJsxAttributesAssignableToTagNameAttributes(node);
}

function getJsxNamespaceSymbol(location: ts.Node, diagnostics: boolean): Symbol {
const reactRefErr = diagnostics && compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
const reactNamespace = getJsxNamespace();
return resolveName(location, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace);
}

/**
* Check if a property with the given name is known anywhere in the given type. In an object type, a property
* is considered known if the object type is empty and the check is for assignability, if the object type has
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2594,6 +2594,8 @@ namespace ts {
* Does not include properties of primitive types.
*/
/* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[];

/* @internal */ getJsxNamespaceSymbol(location: ts.Node): Symbol;
}

export enum NodeBuilderFlags {
Expand Down
8 changes: 8 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2004,6 +2004,10 @@ namespace ts {
return node.kind === SyntaxKind.NumericLiteral;
}

export function isStringLiteral(node: Node): node is StringLiteral {
return node.kind === SyntaxKind.StringLiteral;
}

export function isStringOrNumericLiteral(node: Node): node is StringLiteral | NumericLiteral {
const kind = node.kind;
return kind === SyntaxKind.StringLiteral
Expand Down Expand Up @@ -4043,6 +4047,10 @@ namespace ts {
return node.kind === SyntaxKind.ExportAssignment;
}

export function isModuleDeclaration(node: Node): node is ModuleDeclaration {
return node.kind === SyntaxKind.ModuleDeclaration;
}

export function isModuleOrEnumDeclaration(node: Node): node is ModuleDeclaration | EnumDeclaration {
return node.kind === SyntaxKind.ModuleDeclaration || node.kind === SyntaxKind.EnumDeclaration;
}
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
37 changes: 26 additions & 11 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,22 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

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

else next line, turn on the linter on here please

Copy link
Author

Choose a reason for hiding this comment

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

D'oh, linter was doing nothing. #16078

// 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.getJsxNamespaceSymbol(token));
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 +173,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 +232,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 +389,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 +406,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 +426,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
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([]);