GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add jsxFactory compiler option. by bradleyayers · Pull Request #11267 · microsoft/TypeScript

Conversation

@bradleyayers

@bradleyayers Bradley Ayers (bradleyayers) commented Sep 29, 2016

Copy link
Copy Markdown

Fixes #9582

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
treshugart, stevemao, svieira, styfle, nozzlegear, alex-kinokon, tbekolay, and mvladic reacted with hooray emoji
All reactions
  • 🎉 8 reactions
@bradleyayers
Add jsxFactory compiler option.
63fed8f
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Sep 29, 2016

Copy link
Copy Markdown

Hi Bradley Ayers (@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;
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-signed label Sep 29, 2016
@bradleyayers Bradley Ayers (bradleyayers) mentioned this pull request Sep 29, 2016
Support Custom JSX Pragma #9582
Closed
mhegazy
Mohamed Hegazy (mhegazy) suggested changes Sep 30, 2016
View reviewed changes
Comment thread src/compiler/program.ts Outdated
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Invalid_value_for_reactNamespace_0_is_not_a_valid_identifier, options.reactNamespace));
}

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

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/factory.ts Outdated

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

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradleyayers Bradley Ayers (bradleyayers) Sep 30, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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 Ryan Cavanaugh (@RyanCavanaugh)'s comment:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradleyayers Bradley Ayers (bradleyayers) Sep 30, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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 Ryan Cavanaugh (@RyanCavanaugh) to chime in.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Sep 30, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 Mohamed Hegazy (@mhegazy) ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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 Mohamed Hegazy (@mhegazy) ?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradleyayers Bradley Ayers (bradleyayers) Oct 2, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/factory.ts Outdated
return createIdentifier(jsxFactory);
}

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

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts Outdated
const jsxFactoryRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
let jsxFactoryNamespace = "React";
if (compilerOptions.jsxFactory) {
jsxFactoryNamespace = compilerOptions.jsxFactory.split(".")[0];

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradleyayers Bradley Ayers (bradleyayers) Sep 30, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/conformance/jsx/tsxReactEmitJsxFactory4.tsx
@@ -0,0 +1,12 @@
//@filename: file.tsx
//@jsx: react
//@jsxFactory: a.b.c

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ryan Cavanaugh (@RyanCavanaugh) are dotted names allowed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradleyayers Bradley Ayers (bradleyayers) Oct 2, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bradleyayers
Require 'jsx' if 'jsxFactory' is used and other tidy ups.
3076add
@bradleyayers Bradley Ayers (bradleyayers) mentioned this pull request Sep 30, 2016
create a standalone typing module and flowtype module skatejs/skatejs#754
Closed
Bradley Ayers (bradleyayers) added 2 commits September 30, 2016 19:05
@bradleyayers
Put diagnostic messages in the correct file.
db9d0ff
@bradleyayers
merge master
71b8f00
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) requested changes Sep 30, 2016
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
const jsxFactoryRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined;
let jsxFactoryNamespace = "React";
if (compilerOptions.jsxFactory) {
jsxFactoryNamespace = compilerOptions.jsxFactory.split(".")[0];

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Sep 30, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 . / [

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Sep 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bradleyayers reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Bradley Ayers (bradleyayers) added 2 commits October 2, 2016 19:34
@bradleyayers
Validate jsxFactory as an EntityName.
ceda48b
@bradleyayers
Refactor factory:jsxFactory to create granular nodes for entity name.
49ee64d
bradleyayers
Bradley Ayers (bradleyayers) commented Oct 2, 2016
View reviewed changes
Comment thread src/compiler/factory.ts
}

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

@bradleyayers Bradley Ayers (bradleyayers) Oct 2, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Oct 3, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Anders Hejlsberg (@ahejlsberg) thoughts?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bradleyayers
Bradley Ayers (bradleyayers) commented Oct 2, 2016
View reviewed changes
Comment thread src/compiler/types.ts
inlineSources?: boolean;
isolatedModules?: boolean;
jsx?: JsxEmit;
jsxFactory?: string;

@bradleyayers Bradley Ayers (bradleyayers) Oct 2, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Oct 3, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
rbuckton
Ron Buckton (rbuckton) suggested changes Oct 4, 2016
View reviewed changes
Comment thread src/compiler/checker.ts
while (entityName.kind === SyntaxKind.QualifiedName) {
entityName = (<QualifiedName>entityName).left;
}
Debug.assertNode(entityName, node => node.kind === SyntaxKind.Identifier);

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/parser.ts
return result;
}

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

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/parser.ts

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

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts
inlineSources?: boolean;
isolatedModules?: boolean;
jsx?: JsxEmit;
jsxFactory?: string;

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/factory.ts
}

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

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/parser.ts
const entityName = parseEntityName(allowReservedWords);
parseExpected(SyntaxKind.EndOfFileToken);

const diagnostics = parseDiagnostics;

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/transformers/jsx.ts
const element = createReactCreateElement(
compilerOptions.reactNamespace,
const jsxFactory = compilerOptions.jsxFactory
? createJsxFactory(compilerOptions.jsxFactory)

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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) {

@rbuckton Ron Buckton (rbuckton) Oct 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Steve Mao (stevemao) added a commit to stevemao/awesome-pull-requests that referenced this pull request Oct 5, 2016
@stevemao
add microsoft/TypeScript#11267
767d738
bradleyayers
Bradley Ayers (bradleyayers) commented Oct 9, 2016
View reviewed changes
Comment thread 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

@bradleyayers Bradley Ayers (bradleyayers) Oct 9, 2016

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@wgv-sethlivingston

Seth Livingston (wgv-sethlivingston) commented Oct 27, 2016

Copy link
Copy Markdown

poke. poke-poke. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@notoriousb1t B1t (notoriousb1t) mentioned this pull request Nov 7, 2016
Add "local" option to "JSX" to support VueJS TSX files #12067
Closed
@ashaffer

Andrew Shaffer (ashaffer) commented Nov 9, 2016

Copy link
Copy Markdown

Poke. I'd like this too.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhegazy

Mohamed Hegazy (mhegazy) commented Nov 10, 2016

Copy link
Copy Markdown
Contributor

This should be fixed by #12135
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhegazy Mohamed Hegazy (mhegazy) closed this Nov 10, 2016
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) RyanCavanaugh requested changes
+2 more reviewers
@mhegazy Mohamed Hegazy (mhegazy) mhegazy requested changes
@rbuckton Ron Buckton (rbuckton) rbuckton requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@bradleyayers @msftclas @wgv-sethlivingston @ashaffer @mhegazy @rbuckton @RyanCavanaugh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.