Add jsxFactory compiler option. by bradleyayers · Pull Request #11267 · microsoft/TypeScript

bradleyayers · 2016-09-29T23:55:00Z

msftclas · 2016-09-29T23:55:04Z

Hi @bradleyayers, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

mhegazy · 2016-09-30T00:17:00Z

src/compiler/program.ts

                programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Invalid_value_for_reactNamespace_0_is_not_a_valid_identifier, options.reactNamespace));
            }

+            if (options.jsxFactory && options.reactNamespace) {


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

mhegazy · 2016-09-30T00:20:38Z

src/compiler/factory.ts


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


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

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

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

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

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

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.

mhegazy · 2016-09-30T00:21:43Z

src/compiler/factory.ts

+        return createIdentifier(jsxFactory);
+    }
+
+    export function createReactCreateElement(reactNamespace: string, parentElement: JsxOpeningLikeElement) {


make reactNamespace optional, or string|undefined.

mhegazy · 2016-09-30T00:23:45Z

src/compiler/checker.ts

+            const jsxFactoryRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
+            let jsxFactoryNamespace = "React";
+            if (compilerOptions.jsxFactory) {
+                jsxFactoryNamespace = compilerOptions.jsxFactory.split(".")[0];


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

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.

mhegazy · 2016-09-30T00:24:47Z

tests/cases/conformance/jsx/tsxReactEmitJsxFactory4.tsx

@@ -0,0 +1,12 @@
+//@filename: file.tsx
+//@jsx: react
+//@jsxFactory: a.b.c


@RyanCavanaugh are dotted names allowed?

well.. i suppose if you pass React.createElement it should just work.

OK, so we need to verify it is a qualified name. we do not want unexpected characters in there.

RyanCavanaugh · 2016-09-30T00:04:42Z

src/compiler/checker.ts

+            const jsxFactoryRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
+            let jsxFactoryNamespace = "React";
+            if (compilerOptions.jsxFactory) {
+                jsxFactoryNamespace = compilerOptions.jsxFactory.split(".")[0];


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

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

bradleyayers · 2016-10-02T11:43:57Z

src/compiler/factory.ts

    }

-    function createReactNamespace(reactNamespace: string, parent: JsxOpeningLikeElement) {
+    export function createEntityName(entityName: string) {


I'm skeptical the implementation of this is ideal, open to suggestions.

@ahejlsberg thoughts?

Perhaps a parseIsolatedEntityName could set a flag in the parser to have it treat the source as synthetic.

bradleyayers · 2016-10-02T11:45:59Z

src/compiler/types.ts

        inlineSources?: boolean;
        isolatedModules?: boolean;
        jsx?: JsxEmit;
+        jsxFactory?: string;


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?

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

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

rbuckton · 2016-10-04T00:15:21Z

src/compiler/checker.ts

+                while (entityName.kind === SyntaxKind.QualifiedName) {
+                    entityName = (<QualifiedName>entityName).left;
+                }
+                Debug.assertNode(entityName, node => node.kind === SyntaxKind.Identifier);


There is already an isIdentifier node test you can pass to Debug.assertNode.

rbuckton · 2016-10-04T00:19:37Z

src/compiler/parser.ts

            return result;
        }

+        export function parseIsolatedEntityName(content: string, allowReservedWords = false) {


Is allowReservedWords ever used by a caller? If not, I would just remove it.

rbuckton · 2016-10-04T00:21:28Z

src/compiler/parser.ts


+    /* @internal */
+    export function parseIsolatedEntityName(content: string) {
+        const result = Parser.parseIsolatedEntityName(content);


Parent references aren't used in the emit transforms with respect to synthesized nodes. If this is only used in emit, then fixing up the parent references is unneeded.

rbuckton · 2016-10-04T00:31:19Z

src/compiler/types.ts

        inlineSources?: boolean;
        isolatedModules?: boolean;
        jsx?: JsxEmit;
+        jsxFactory?: string;


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

rbuckton · 2016-10-04T00:38:13Z

src/compiler/factory.ts

    }

-    function createReactNamespace(reactNamespace: string, parent: JsxOpeningLikeElement) {
+    export function createEntityName(entityName: string) {


Perhaps a parseIsolatedEntityName could set a flag in the parser to have it treat the source as synthetic.

rbuckton · 2016-10-04T00:41:22Z

src/compiler/parser.ts

+            const entityName = parseEntityName(allowReservedWords);
+            parseExpected(SyntaxKind.EndOfFileToken);
+
+            const diagnostics = parseDiagnostics;


Are you using these diagnostics, other than to check that they exist in program.ts:1546? You could return undefined rather than allocating the object literal for every call.

rbuckton · 2016-10-04T00:43:23Z

src/compiler/transformers/jsx.ts

-            const element = createReactCreateElement(
-                compilerOptions.reactNamespace,
+            const jsxFactory = compilerOptions.jsxFactory
+                ? createJsxFactory(compilerOptions.jsxFactory)


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.

rbuckton · 2016-10-04T00:46:18Z

src/compiler/checker.ts

+            // 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) {


This could be done once during initializeTypeChecker rather than on every call to checkJsxOpeningLikeElement.

bradleyayers · 2016-10-09T06:59:05Z

src/compiler/factory.ts

+    }
+
+    export function createReactCreateElement(reactNamespace: string | undefined, parentElement: JsxOpeningLikeElement) {
        // To ensure the emit resolver can properly resolve the namespace, we need to


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.

wgv-sethlivingston · 2016-10-27T14:29:20Z

poke. poke-poke. :)

ashaffer · 2016-11-09T20:03:16Z

Poke. I'd like this too.

mhegazy · 2016-11-10T18:12:17Z

This should be fixed by #12135

Add jsxFactory compiler option.

63fed8f

msftclas added the cla-signed label Sep 29, 2016

bradleyayers mentioned this pull request Sep 29, 2016

Support Custom JSX Pragma #9582

Closed

mhegazy suggested changes Sep 30, 2016

View reviewed changes

Require 'jsx' if 'jsxFactory' is used and other tidy ups.

3076add

bradleyayers mentioned this pull request Sep 30, 2016

create a standalone typing module and flowtype module skatejs/skatejs#754

Closed

bradleyayers added 2 commits September 30, 2016 19:05

Put diagnostic messages in the correct file.

db9d0ff

merge master

71b8f00

RyanCavanaugh requested changes Sep 30, 2016

View reviewed changes

bradleyayers added 2 commits October 2, 2016 19:34

Validate jsxFactory as an EntityName.

ceda48b

Refactor factory:jsxFactory to create granular nodes for entity name.

49ee64d

bradleyayers commented Oct 2, 2016

View reviewed changes

rbuckton suggested changes Oct 4, 2016

View reviewed changes

stevemao added a commit to stevemao/awesome-pull-requests that referenced this pull request Oct 5, 2016

add microsoft/TypeScript#11267

767d738

bradleyayers commented Oct 9, 2016

View reviewed changes

notoriousb1t mentioned this pull request Nov 7, 2016

Add "local" option to "JSX" to support VueJS TSX files #12067

Closed

mhegazy closed this Nov 10, 2016

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

bradleyayers commented Sep 29, 2016

Uh oh!

msftclas commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgv-sethlivingston commented Oct 27, 2016

Uh oh!

ashaffer commented Nov 9, 2016

Uh oh!

mhegazy commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!