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

JSX Support #2673

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
@fdecampredon
Copy link

fdecampredon commented Apr 8, 2015

Following the discussion in #296, here is a PR that adds support for JSX constructs.

And some details about the implementation/spec :

  • JSX elements are only parsed if a triple slash directive is present :
///<jsx factory="myFactory" />
  • JSX elements are type checked as a call of the factory function :
///<jsx factory="myFactory" />
<MyComp attr={something}> Something </MyComp>;
// typechecked as : 
myFactory(MyComp, { attr: something}, " Something ");
  • JSX elements are emitted unchanged (only formatted).
  • To reproduce React's behavior, depending of the case of the tag, the first argument of the function call might be treated as a string:
///<jsx factory="myFactory" />
<div />
// typechecked as : 
myFactory("div", {});

<MyComp />
// typechecked as : 
myFactory(MyComp, {});
  • JSX directive factory attribute value might contain any property access level:
///<jsx factory="myNameSpace.myNameSpace1.myFunc" />
///<jsx factory="React.createElement" />
  • In General JSX elements describe how a component should be instantiated (at least in React). In this context the way of how attributes should be checked is generally dictated by the tag.
    For this purpose, types for JSXElement call are only inferred from the first argument (the tag).
///<jsx factory="test" />

declare function test<P>(p1: P, p2: P): void;

var Comp = { items: "foo" };
test(Comp, {}); // no error P is inferred as `{}` 
<Comp />; // error Argument of type '{}' is not assignable to parameter of type '{ items: string; }'.

The service integration is minimal and ported from my old version of jsx-typescript, so not well tested.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Apr 8, 2015

Hi @fdecampredon, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas

This comment has been minimized.

Copy link

msftclas commented Apr 8, 2015

@fdecampredon, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@msftclas msftclas added cla-signed and removed cla-required labels Apr 8, 2015

@paulvanbrenk

This comment has been minimized.

Copy link
Contributor

paulvanbrenk commented Apr 9, 2015

Thanks for the PR, I'll take a look later tomorrow.

@jbondc

This comment has been minimized.

Copy link
Contributor

jbondc commented Apr 9, 2015

@fdecampredon Great stuff

I'm not sure how, but it would be nice to see this solved as general template string:

///<template name="jsx" compile="myFactory" />
jsx`<div />`
// typechecked as : 
myFactory("div", {});
@@ -316,6 +316,20 @@ module ts {
return visitNode(cbNode, (<ExternalModuleReference>node).expression);
case SyntaxKind.MissingDeclaration:
return visitNodes(cbNodes, node.decorators);
case SyntaxKind.JSXElement:
return visitNode(cbNode, (<JSXElement>node).openingElement) || visitNodes(cbNodes, (<JSXElement>node).children) || visitNode(cbNode, (<JSXElement>node).closingElement);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

wrap these to keep things consistent with the rest of this function.

@@ -1105,6 +1121,9 @@ module ts {
// attached to the EOF token.
let parseErrorBeforeNextFinishedNode: boolean = false;

let inJSXTag = false;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

These concern me. I would far prefer less context in our parser/scanner. I presume this is to change the scanning of identifiers in xml tags? If so, why not just scan out an identifier and then decide if its' legal or not if it has something like a unicode escape in it.

If it's for another purpose let me know.

This comment has been minimized.

@fdecampredon

fdecampredon Apr 9, 2015

Yes it is only for identifier scanning in xml tags. I will try to move the validation of this identifier in the parser.

@@ -1741,6 +1777,8 @@ module ts {
return token === SyntaxKind.GreaterThanToken || token === SyntaxKind.OpenParenToken;
case ParsingContext.HeritageClauses:
return token === SyntaxKind.OpenBraceToken || token === SyntaxKind.CloseBraceToken;
case ParsingContext.JSXAttributes:
return token === SyntaxKind.GreaterThanToken || token === SyntaxKind.SlashToken;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

Presumably also close brace?

This comment has been minimized.

@fdecampredon

fdecampredon Apr 9, 2015

Why ? close brace is an invalid token in attributes list, not a list terminator. Is there something I misunderstood ?

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

You use "token === SyntaxKind.OpenBraceToken" to determine if you need to start parsing JSXAttributes. IsListterminator is called if we're in error recovery and we need to stop parsing whatever construct we're currently in. If we're in error recovery and we see a } then presumably we want to stop parsing the current JSXAttributes we're in.

@@ -3480,7 +3518,7 @@ module ts {
case SyntaxKind.VoidKeyword:
return parseVoidExpression();
case SyntaxKind.LessThanToken:
return parseTypeAssertion();
return (sourceFile.jsxFactory && tryParse(() => parseJSXElement(true))) || parseTypeAssertion();

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

this is going to be a big hit given that we do tihs on every type assertion. We'll need to preprocess the file to determine what the set of possible close tag names are (i.e. search for all occurrences of ""). Then, based on that table, we can determine if we need to do the costly parseJSX path or not.

This comment has been minimized.

@fdecampredon

fdecampredon Apr 9, 2015

At first I had a preprocessing phase that was building a map of closing tags to improve performance like you described.
The problem with that approach is that it produces very unfriendly errors in case of typo in closing tag.
I so then switched to a simple array of closing tag positions, and I checked in case of non self closing element if there was at least one closing tag later in the file.
However I thought that this optimisation was complex for very few results in this PR since I only parse JSX elements if the file contains the triple slash directive. (and that there will certainly be closing tags in this case)

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

CAn you give examples where you get unfriendly errors?

Even if we're only parsing JSX elmeents in .tsx files (or in the presence of this directive) it may still be very impactful on perf to do this. We would still want people to be able to edit large .tsx files in the IDE without parsing being super expensive.

I'm also worried about the parsing cost because this lookahead is exponentially bad. i.e. you can be speculatively parsing one element, then you have to do that again inside the element. Then if that outer element fails to parse, then we do that work again when we hit the inner element.

This comment has been minimized.

@fdecampredon

fdecampredon Apr 10, 2015

in this case for example:

<div> Some jsx text { someExpression}  ....... </dv> 

If we look for a closing </div> tag before parsing the content of the element for validating that <div> is not a type assertion, everything inside the element, and especially JSX text, will be parsed as normal code, which generally lead in a lot of errors, and not reported at the right place, when the true error is that there is a typo in the closing element tag name.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 10, 2015

Contributor

I definitely see what you're saying. However, i'm still very concerned here as the current approach means that every type assertion in a .tsx file will cause this potentially unbounded lookahead. Once you're in an JSXChild context your scanner will keep on returning JSXText until it hits a {, }, < or </. And only } and </ actually cause us to stop parsing text. As such, it would be possible for us to scan huge swaths of code before finally deciding that it isn't a JSXElement, and trying again as a type assertion. And, unfortunately, because you can easily have lots of code with nested type assertions, we'll be doing that lookahead over and over again.

Now: i totally get the concern for the case you've mentioned. So there are approaches we can take to mitigate this issue and hopefully provide good perf, and better handling of code like what you've written above. First, we would use the close-tag table as outlined above. This would allow us to parse efficiently in the case where you had a correctly written close tag. However we could then augment that slightly. Specifically, if we run into a <tag> and we don't see a </tag> in our table, then we can look ahead a bit to see if what follows is a valid unary expression sequence. If it isn't (say because you have <tag>foo bar, then we go ahead and still parse it as an XML literal.

There is precedence because this is how we handle certain things like yield. Even if you're not in a generator function, but we see something like yield a, then we realize "Hey! treating yield as an identifier here won't succeed, and will likely produce worse code. So let's just parse out the actual yield-expression, and give a good message that it's not legal outside of a generator." Such an approach could be viable here. Now instead of unbounded lookahead in all cases, we do limited lookahead in the cases where we don't see a viable closing tag.

@@ -605,12 +610,23 @@ module ts {
}

function isIdentifierStart(ch: number): boolean {
if (inJSXTag && ch === CharacterCodes.backslash) {

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

I would avoid this. Instead, just have the scanner track if it ran into a unicode escape while scanning an identiifer. If so, we can either not allow the token to be used at hte parser level for a JSX tag. Or, we can allow it, and we can appropriately emit hte cooked name instead of the raw name.

if (ch === CharacterCodes.backslash) {
return false;
}
else if (ch === CharacterCodes.minus) {

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

I would not do things this way. Instead, we should either go the 'rescanXXX' route (like we do for regex/templates), or we should just scan these out a sequence of 'identifier + minus ...' tokens that the parser can accept as the name of an xml tag.


if (inJSXChild) {
switch (ch) {
case CharacterCodes.closeBrace:

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

why do you need these first two cases?

This comment has been minimized.

@fdecampredon

fdecampredon Apr 9, 2015

closeBrace need to be caught to disambiguate case like:

<div> {<div>{}} </div>

In speculative JSX element parsing the scanner will return an invalid closeBrace token during the parsing of the second <div> and parseJSXElement will return null.
see: https://github.com/fdecampredon/jsx-typescript/blob/jsx-dev/src/compiler/parser.ts#L3693-L3699

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

I guess I don't see how these two cases are different from how we treat closebrace/openbrace in the switch below.

This comment has been minimized.

@fdecampredon

fdecampredon Apr 10, 2015

I'm sorry but I don't understand the direction you're pointing me here.
When in jsx child, the normal scanning process is shunted and the only token handled are {,},< and </ (we might add > for coherence). Everything else is put in a single JSXText token.
Would you prefer than I just break the switch for open and close brace and let the normal process take place ?

case CharacterCodes.openBrace:
return pos++, token = SyntaxKind.OpenBraceToken;
case CharacterCodes.lessThan:
if (text.charCodeAt(pos + 1) === CharacterCodes.slash) {

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Apr 9, 2015

Contributor

Interesting ambiguity with: something </regex/.Something()... Will have to consider the implications of this.

This comment has been minimized.

@fdecampredon

fdecampredon Apr 9, 2015

in case of something like :

<foo>test </foo/.Something()

We could try something with closing tag parsing, for example if we detect an / instead of the last > in the closing element parseJSXElement could return null when speculative ?

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Apr 9, 2015

I'm concerned about the incremental parsing issues here. If you have:

var v = <book>x;
var y;
var z;

And you add something at the end like </book>, then we won't know to go reparse that original statement.

We'll need to track (in the parse tree) where type assertions are used. And then, during incremental parsing, we'll have to walk into anything that has a type assertion, and see if that type assertion could match a close tag that follows it in the file. If so, we'll have to consider the node ineligible for incremental reuse.

@fdecampredon

This comment has been minimized.

Copy link

fdecampredon commented Apr 9, 2015

@CyrusNajmabadi
Do you want that I try to address the Incremental parsing issue in this PR, or do it later in a separate PR if that one is merged ?

@paulvanbrenk

This comment has been minimized.

Copy link
Contributor

paulvanbrenk commented Apr 9, 2015

The typechecking using the factory seems problematic as a general solution, and there is no clean consensus at this point what the right direction is... can you remove the type-checking from the PR and for now type things as 'any'.

I'm aware of the limitations, and agree we should do something else in the future. Just not sure what...

@fdecampredon

This comment has been minimized.

Copy link

fdecampredon commented Apr 9, 2015

any or {} ? because that was the 2 options described by @mhegazy here #296 (comment).
And any seems to have big impact of what could be done in the future.

@paulvanbrenk

This comment has been minimized.

Copy link
Contributor

paulvanbrenk commented Apr 9, 2015

any or {} which ever is easier for you to implement.

We should come up with a design for typing things before we actually release this and make changes based on that. But we should focus on a solid (and performant) parsing story first. First walk, then run etc...

Thanks for all the work!

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Apr 10, 2015

@fdecampredon

Do you want that I try to address the Incremental parsing issue in this PR, or do it later in a separate PR if that one is merged ?

I would definitely want the incremental parsing issue resolved before merging. Otherwise, we could end up in a state where you're editing, but invalid parse trees. We want to avoid that like the plague, else we will have many hard to track down bugs.

Here would be my preferred order of work:

  1. Get parsing fully complete. Have system report that JSX elements are not supported during typecheck, treat them as 'any' in the typesystem, and skip processing them further. Ignore JSX elements for emit.
  2. Get emit working. I would hope this would be a fairly simple task.
  3. Come to consensus with how this should be properly represented in the Type-system (will likely require language design approval), then do the type checking side.

This allows the work to be spread into much smaller pieces, and allows us to make sure that things are working properly before going on to the next stages.

Thanks!

@chicoxyzzy

This comment has been minimized.

Copy link
Contributor

chicoxyzzy commented Apr 17, 2015

also notice that some tags should omit their close tag

@duanyao

This comment has been minimized.

Copy link

duanyao commented Apr 18, 2015

How about using double angle brackets for XML-like syntax in typescript? This should avoids conflict with type assertion:

<<div id=1>>
  var text = (<Element>document.querySelector("#foo")).textContent;
  ...
<</div>>

I think even if the TS compiler/IDEs are smart enough to recognize single-angle-bracket XML tags, it is harder for human eyes to quickly distinguish single-angle-bracket XML tags and type assertions and generics.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Apr 18, 2015

How about using double angle brackets for XML-like syntax in typescript

Too verbose. There have been other simpler recommendations like <?div> where only the first instance needs changing.

That said I strongly prefer what @fdecampredon has done and it would be awesome to have in TS core.

@duanyao

This comment has been minimized.

Copy link

duanyao commented Apr 18, 2015

@basarat Yes double angle bracket is verbose, but personally I think it is more pretty-looking than something like <?div>. Besides, single angle brackets are already used for type assertion, generics, and comparisons in TS, and double angle bracket makes it easier to recognize XML tags by human eyes. We usually read codes more times than write them, so readability of the syntax is more important than writability, I think.

P.S., smart code editors may autocomplete double angle brackets, e.g., after you type <div>, editors can generate <<div>><</div>>, so the verbosity may be not a big problem.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Apr 18, 2015

Did you imagine every level or just first level?

<<div>>
    <<div>>
    <</div>>
<</div>>
@duanyao

This comment has been minimized.

Copy link

duanyao commented Apr 18, 2015

Probably every level, because TS codes may be mixed inside XML elements.

@BohdanTkachenko

This comment has been minimized.

Copy link

BohdanTkachenko commented May 1, 2015

Any chances that TypeScript will get official support and this PR will be merged? I want to migrate my project to TypeScript, but JSX support is essential for me.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented May 1, 2015

last time I chatted with @fdecampredon he was busy moving, So hopefully we can get in soon :)

@icetraxx

This comment has been minimized.

Copy link

icetraxx commented May 1, 2015

+1 on getting this in soon.

@DAddYE

This comment has been minimized.

Copy link

DAddYE commented May 4, 2015

I would consider Typescript as well for my next project if it can support JSX. So please asap! 👍

@paulvanbrenk

This comment has been minimized.

Copy link
Contributor

paulvanbrenk commented May 4, 2015

I want to set some expectations here. We won't be able to get this in the 1.5 release, however we're moving as fast as we can.

@tinganho

This comment has been minimized.

Copy link
Contributor

tinganho commented May 5, 2015

@fdecampredon could you please look at #3022 ? I would like to have your thoughts on it. I know that you commented before that you didn't expect the TS team to include JSX and thus created your own fork because you thought it "was the right way", seems like they changed their mind now.

@icetraxx

This comment has been minimized.

Copy link

icetraxx commented May 6, 2015

@fdecampredon @CyrusNajmabadi How about using .jsx.ts instead of .tsx for the file extension? This is more consistent with what TypeScript already does with d.ts files (as proposed by @jonathandturner here #3022 (comment)) and doesn't add an entirely new file extension.

@fdecampredon

This comment has been minimized.

Copy link

fdecampredon commented May 7, 2015

@CyrusNajmabadi @paulvanbrenk would you mind if I completely rewrite the git history, it's a bit a mess now and a lot of things has been added then removed (especially in scanner).

@koistya

This comment has been minimized.

Copy link

koistya commented May 7, 2015

Would be nice if it could also work with .react.ts as well as the default .ts file extensions.

@DAddYE

This comment has been minimized.

Copy link

DAddYE commented May 7, 2015

I'll rather prefer .tsx or jsx.ts

Sent from my iPhone

On May 7, 2015, at 4:56 AM, Konstantin Tarkus notifications@github.com
wrote:

Would be nice if it could also work with .react.ts as well as the default
.ts file extensions.


Reply to this email directly or view it on GitHub
#2673 (comment).

@Lenne231

This comment has been minimized.

Copy link

Lenne231 commented May 8, 2015

Is there a good reason to have a special file extension? I don't think so, if it's part of TypeScript it should be allowed to use JSX in every *.ts file. JSX uses *.jsx to differentiate from *.js but TypeScript is allready differentiated from *.js by using *.ts, so there is no need for a special file extension...

@majodev

This comment has been minimized.

Copy link

majodev commented May 13, 2015

Really looking forward for this!

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 15, 2015

I'm ok with you rewriting things. @paulvanbrenk are you ok with this?

@paulvanbrenk

This comment has been minimized.

Copy link
Contributor

paulvanbrenk commented May 15, 2015

Go right ahead.

@fdecampredon fdecampredon force-pushed the fdecampredon:jsx-dev branch from 0b673c2 to 8d88965 May 19, 2015

@fdecampredon

This comment has been minimized.

Copy link

fdecampredon commented May 19, 2015

@paulvanbrenk @CyrusNajmabadi I tried to address all your feedbacks except for incremental parsing.
However from what I read in #3203 things have changed.
What do you want that I do ?
Should I wait for the as operator to be merged and remove everything that try to disambiguate JSX from type assertion ? Or simply finish it like that ?
@CyrusNajmabadi do you want to explain me or in details what should be done for incremental parsing, or implement it yourself ?

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 19, 2015

Hi @fdecampredon . I would wait on 'as' to get merged in, and i would remove all the disambiguation logic you have. With TSX, we'll be requiring that you use 'as' for type assertions, and things like <blah> will always be xml tags.

This does mean that .ts code can't simply be dropped into a .tsx file, but c'est la vie.

Because of these changes, i don't believe there is any work you will need to do for incremental parsing anymore. The problem with incremental parsing was the ambiguity with <tags>. Basically, we would have had to have logic that would say "after an edit, we need to know if we need to reinpterpret any preceding type-assertions as xml-tags". Now, because there is no ambiguity, we don't have to do this work.

So yaay, everything gets simpler.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jun 18, 2015

This work has been folded in to a future PR

@Microsoft Microsoft locked and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.