Skip to content
Closed
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
19 changes: 12 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10713,13 +10713,18 @@ namespace ts {
checkGrammarJsxElement(node);
checkJsxPreconditions(node);

// The reactNamespace symbol should be marked as 'used' so we don't incorrectly elide its import. And if there
// is no reactNamespace 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 = compilerOptions.reactNamespace ? compilerOptions.reactNamespace : "React";
const reactSym = resolveName(node.tagName, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace);
if (reactSym) {
getSymbolLinks(reactSym).referenced = true;
// The JSX factory namespace symbol should be marked as 'used' so we don't incorrectly elide its import. And if
// it isn't in scope when targeting React emit, we should issue an error.
const jsxFactoryRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
let jsxFactoryNamespace = "React";
if (compilerOptions.jsxFactory) {
jsxFactoryNamespace = compilerOptions.jsxFactory.split(".")[0];
Copy link
Member

Choose a reason for hiding this comment

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

This could also be an expression like foo["bar"], should be able to split on . / [

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a qualified name is sufficient here. we could relax that in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we say jsxFactory is an identifier, no need to do this here.

Copy link
Author

Choose a reason for hiding this comment

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

I was using treating jsxFactory as an Identifier in factory.ts as a convenience in the a.b.c case, but really in that scenario it should be a PropertyAccessExpression.

In that case I only want a out of a.b.c so that I can set referenced = true on that symbol alone.

} else if (compilerOptions.reactNamespace) {
jsxFactoryNamespace = compilerOptions.reactNamespace;
}
const jsxFactorySym = resolveName(node.tagName, jsxFactoryNamespace, SymbolFlags.Value, jsxFactoryRefErr, jsxFactoryNamespace);
if (jsxFactorySym) {
getSymbolLinks(jsxFactorySym).referenced = true;
}

const targetAttributesType = getJsxElementAttributesType(node);
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ namespace ts {
paramType: Diagnostics.KIND,
description: Diagnostics.Specify_JSX_code_generation_Colon_preserve_or_react,
},
{
name: "jsxFactory",
type: "string",
description: Diagnostics.Specify_the_JSX_factory_function_to_use_when_targeting_react_JSX_emit_eg_React_createElement_or_h,
},
{
name: "reactNamespace",
type: "string",
Expand Down
19 changes: 11 additions & 8 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1618,17 +1618,23 @@ namespace ts {
);
}

function createReactNamespace(reactNamespace: string, parent: JsxOpeningLikeElement) {
export function createJsxFactory(jsxFactory: string) {
// No explicit validation of this parameter is required. Users are
Copy link
Contributor

Choose a reason for hiding this comment

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

do not think you can do this. in Program:verifyCompilerOptions, you should check that isIdentifierText(options.jsxFactory, languageVersion)

Copy link
Author

Choose a reason for hiding this comment

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

I went with this approach for simplicity, based on the belief this was acceptable behaviour as per @RyanCavanaugh's comment:

No explicit validation of this parameter occurs; users are assumed to have provided a correct string

Copy link
Author

Choose a reason for hiding this comment

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

To avoid going around in circles I'll wait for @RyanCavanaugh to chime in.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just restrict this to dotted names for simplicity's sake. Thoughts @mhegazy ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just restrict this to dotted names for simplicity's sake. Thoughts @mhegazy ?

I agree. let's add a check that this is a QualifiedName in Program:verifyCompilerOptions

Copy link
Author

Choose a reason for hiding this comment

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

I need some help with deciding what strategy is best for this.

To validate a dotted name as a QualifiedName, there’s a few ways I can see to do it:

  • basic regex or string manipulation (e.g. .split(‘.’))
  • creating a mini parser using scanner, iterating over the tokens and asserting it’s a sequence of Identifier (Dot, Identifier)*
  • using the parser (createSourceFile) to parse the string a.b.c and using the resulting source file to both assert the structure, and to extract the a identifier to mark it as used
  • …?

Balancing simplicity with correctness is the challenge here, I'm not sure what the preferred bias is.

// assumed to have provided a correct string.
return createIdentifier(jsxFactory);
}

export function createReactCreateElement(reactNamespace: string, parentElement: JsxOpeningLikeElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

make reactNamespace optional, or string|undefined.

// To ensure the emit resolver can properly resolve the namespace, we need to
Copy link
Author

Choose a reason for hiding this comment

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

I don't really understand this comment. What does "resolving the namespace" by the emit resolver mean, and why does it need to? My guess would have been to ensure that the React symbol is treated as "used" (so that the import is not elided), but this was already done manually in checker.ts:10804, so I must be missing something.

// treat this identifier as if it were a source tree node by clearing the `Synthesized`
// flag and setting a parent node.
const react = createIdentifier(reactNamespace || "React");
react.flags &= ~NodeFlags.Synthesized;
react.parent = parent;
return react;
react.parent = parentElement;
return createPropertyAccess(react, "createElement");
}

export function createReactCreateElement(reactNamespace: string, tagName: Expression, props: Expression, children: Expression[], parentElement: JsxOpeningLikeElement, location: TextRange): LeftHandSideExpression {
export function createJsxFactoryCall(jsxFactory: Identifier | PropertyAccessExpression, tagName: Expression, props: Expression, children: Expression[], parentElement: JsxOpeningLikeElement, location: TextRange): LeftHandSideExpression {
const argumentsList = [tagName];
if (props) {
argumentsList.push(props);
Expand All @@ -1651,10 +1657,7 @@ namespace ts {
}

return createCall(
createPropertyAccess(
createReactNamespace(reactNamespace, parentElement),
"createElement"
),
jsxFactory,
/*typeArguments*/ undefined,
argumentsList,
location
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2346,6 +2346,10 @@ namespace ts {
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Invalid_value_for_reactNamespace_0_is_not_a_valid_identifier, options.reactNamespace));
}

if (options.jsxFactory && options.reactNamespace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should add another check as well that jsxFactory can not be used without jsx: react

programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_with_option_1, "jsxFactory", "reactNamespace"));
}

// If the emit is enabled make sure that every output file is unique and not overwriting any of the input files
if (!options.noEmit && !options.suppressOutputPathCheck) {
const emitHost = getEmitHost();
Expand Down
8 changes: 6 additions & 2 deletions src/compiler/transformers/jsx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,12 @@ namespace ts {
|| createAssignHelper(currentSourceFile.externalHelpersModuleName, segments);
}

const element = createReactCreateElement(
compilerOptions.reactNamespace,
const jsxFactory = compilerOptions.jsxFactory
? createJsxFactory(compilerOptions.jsxFactory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a fully-synthesized entity name, consider caching it on SourceFile, similar to externalHelpersModuleName rather than reparsing it each time it is hit. You could also create it once at the top of transformJsx.

: createReactCreateElement(compilerOptions.reactNamespace, node);

const element = createJsxFactoryCall(
jsxFactory,
tagName,
objectProperties,
filter(map(children, transformJsxChildToExpression), isDefined),
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2701,6 +2701,7 @@ namespace ts {
inlineSources?: boolean;
isolatedModules?: boolean;
jsx?: JsxEmit;
jsxFactory?: string;
Copy link
Author

Choose a reason for hiding this comment

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

Instead of parsing the jsxFactory to an EntityName multiple times through-out the program, would it be better to have this be EntityName and parse it once up-front?

Copy link
Member

Choose a reason for hiding this comment

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

It'd probably be cleanest to do this once in checker.ts and then only again if needed in emitter.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is somewhat similar to what we do for externalHelpersModuleName on SourceFile.

lib?: string[];
/*@internal*/listEmittedFiles?: boolean;
/*@internal*/listFiles?: boolean;
Expand Down
4 changes: 4 additions & 0 deletions src/harness/unittests/transpile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,10 @@ var x = 0;`, {
options: { compilerOptions: { jsx: 1 }, fileName: "input.js", reportDiagnostics: true }
});

transpilesCorrectly("Supports setting 'jsxFactory'", "x;", {
options: { compilerOptions: { jsxFactory: "React.createElement" }, fileName: "input.js", reportDiagnostics: true }
});

transpilesCorrectly("Supports setting 'lib'", "x;", {
options: { compilerOptions: { lib: ["es2015", "dom"] }, fileName: "input.js", reportDiagnostics: true }
});
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
tests/cases/conformance/jsx/file.tsx(8,2): error TS2304: Cannot find name 'h'.


==== tests/cases/conformance/jsx/file.tsx (1 errors) ====
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

// This should raise an error as 'h' is not declared.
<div />;
~~~
!!! error TS2304: Cannot find name 'h'.

14 changes: 14 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [file.tsx]
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

// This should raise an error as 'h' is not declared.
<div />;


//// [file.js]
// This should raise an error as 'h' is not declared.
h("div", null);
14 changes: 14 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [file.tsx]
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

declare var h: any;

<div />;


//// [file.js]
h("div", null);
18 changes: 18 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory2.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : Symbol(JSX, Decl(file.tsx, 0, 0))

interface IntrinsicElements {
>IntrinsicElements : Symbol(IntrinsicElements, Decl(file.tsx, 0, 20))

[s: string]: any;
>s : Symbol(s, Decl(file.tsx, 2, 3))
}
}

declare var h: any;
>h : Symbol(h, Decl(file.tsx, 6, 11))

<div />;
>div : Symbol(JSX.IntrinsicElements, Decl(file.tsx, 0, 20))

19 changes: 19 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory2.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : any

interface IntrinsicElements {
>IntrinsicElements : IntrinsicElements

[s: string]: any;
>s : string
}
}

declare var h: any;
>h : any

<div />;
><div /> : any
>div : any

14 changes: 14 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [file.tsx]
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

declare var React: any;

<div />;


//// [file.js]
React.createElement("div", null);
18 changes: 18 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory3.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : Symbol(JSX, Decl(file.tsx, 0, 0))

interface IntrinsicElements {
>IntrinsicElements : Symbol(IntrinsicElements, Decl(file.tsx, 0, 20))

[s: string]: any;
>s : Symbol(s, Decl(file.tsx, 2, 3))
}
}

declare var React: any;
>React : Symbol(React, Decl(file.tsx, 6, 11))

<div />;
>div : Symbol(JSX.IntrinsicElements, Decl(file.tsx, 0, 20))

19 changes: 19 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory3.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : any

interface IntrinsicElements {
>IntrinsicElements : IntrinsicElements

[s: string]: any;
>s : string
}
}

declare var React: any;
>React : any

<div />;
><div /> : any
>div : any

14 changes: 14 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory4.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [file.tsx]
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

declare var a: any;

<div />;


//// [file.js]
a.b.c("div", null);
18 changes: 18 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory4.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : Symbol(JSX, Decl(file.tsx, 0, 0))

interface IntrinsicElements {
>IntrinsicElements : Symbol(IntrinsicElements, Decl(file.tsx, 0, 20))

[s: string]: any;
>s : Symbol(s, Decl(file.tsx, 2, 3))
}
}

declare var a: any;
>a : Symbol(a, Decl(file.tsx, 6, 11))

<div />;
>div : Symbol(JSX.IntrinsicElements, Decl(file.tsx, 0, 20))

19 changes: 19 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory4.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : any

interface IntrinsicElements {
>IntrinsicElements : IntrinsicElements

[s: string]: any;
>s : string
}
}

declare var a: any;
>a : any

<div />;
><div /> : any
>div : any

24 changes: 24 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory5.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//// [tests/cases/conformance/jsx/tsxReactEmitJsxFactory5.tsx] ////

//// [file.tsx]
declare module JSX {
interface IntrinsicElements {
[s: string]: any;
}
}

//// [h.d.ts]
export var h;

//// [react-consumer.tsx]
import {h} from "./h";
// Should not elide h import
<div />;


//// [file.js]
//// [react-consumer.js]
"use strict";
var h_1 = require("./h");
// Should not elide h import
h("div", null);
24 changes: 24 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory5.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : Symbol(JSX, Decl(file.tsx, 0, 0))

interface IntrinsicElements {
>IntrinsicElements : Symbol(IntrinsicElements, Decl(file.tsx, 0, 20))

[s: string]: any;
>s : Symbol(s, Decl(file.tsx, 2, 3))
}
}

=== tests/cases/conformance/jsx/h.d.ts ===
export var h;
>h : Symbol(h, Decl(h.d.ts, 0, 10))

=== tests/cases/conformance/jsx/react-consumer.tsx ===
import {h} from "./h";
>h : Symbol(h, Decl(react-consumer.tsx, 0, 8))

// Should not elide h import
<div />;
>div : Symbol(JSX.IntrinsicElements, Decl(file.tsx, 0, 20))

25 changes: 25 additions & 0 deletions tests/baselines/reference/tsxReactEmitJsxFactory5.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
=== tests/cases/conformance/jsx/file.tsx ===
declare module JSX {
>JSX : any

interface IntrinsicElements {
>IntrinsicElements : IntrinsicElements

[s: string]: any;
>s : string
}
}

=== tests/cases/conformance/jsx/h.d.ts ===
export var h;
>h : any

=== tests/cases/conformance/jsx/react-consumer.tsx ===
import {h} from "./h";
>h : any

// Should not elide h import
<div />;
><div /> : any
>div : any

Loading