[WIP] Clean up version of using overload for JSX stateless function component #12107

yuit · 2016-11-08T19:35:03Z

@sandersn Thanks for reviewing the original PR! This one should make life much easier

… array

Add language service support for JSXAttributes Add completion support Add find-all-references support Add goto-definition support

…onent

sandersn

Comments and questions on the checker. I'll look at the other files next.

sandersn · 2016-11-08T21:27:39Z

src/compiler/checker.ts

+            const paramType = getTypeAtPosition(signature, 0);
+
+            // JSX opening-like element has correct arity for stateless-function component if the one of the following condition is true:
+            //      1. callIsInCompletes


typo:callIsIncomplete #Closed

sandersn · 2016-11-08T21:53:24Z

src/compiler/checker.ts

+            //         We can figure that out by resolving attributes property and check number of properties in the resolved type
+            // If the call has correct arity, we will then check if the argument type and parameter type is assignable
+
+            const callIsIncomplete = node.attributes.end === node.end;  // If we are missing the close "/>", the call is incomplete


Seems like you should move if (callIsIncomplete) right below here to avoid the checkExpression call when possible. #Closed

sandersn · 2016-11-08T21:54:27Z

src/compiler/checker.ts

+                let shouldCheckArgumentAndParameter = true;
+                for (const arg of argProperties) {
+                    if (!getPropertyOfType(paramType, arg.name) && isUnhyphenatedJsxName(arg.name)) {
+                        shouldCheckArgumentAndParameter = false;


can't you just return false here and get rid of the boolean flag? #Closed

sandersn · 2016-11-08T23:01:46Z

src/compiler/checker.ts

+                        break;
+                    }
+                }
+                if (shouldCheckArgumentAndParameter && checkTypeRelatedTo(argType, paramType, relation, /*errorNode*/ undefined, headMessage)) {


this requirement that the types have the same set of properties kind of looks like compareTypesIdentical (except for hyphenated properties). But each property is still compared for normal assignability. This is kind of confusing. Why not just use one-directional assignability that checkTypeRelatedTo gives you? #Pending

sandersn · 2016-11-09T19:43:31Z

src/compiler/checker.ts

@@ -12604,6 +12861,14 @@ namespace ts {
                return result;
            }

+            // Do not report any error if we are doing so for stateless function component as such error will be error will be handle in "resolveCustomJsxElementAttributesType".


replace: "will be error will be handle" with "will be handled" #Closed

It's suspicious to see error handling paths duplicated like this. Can you treat jsx stateless function components close enough to a normal call that you can use the same error paths? So far I have seen three obstacles:

hypenated properties are treated differently

The rules for applicability are (perhaps?) different.

Error reporting on JSX as if it were a function call might be confusing.

Can you think of ways to handle these on the normal code paths instead of putting them in a set of alternate functions? Are there other problems? (3) doesn't seem that hard to fix locally. #Closed

The reason that JSX stateless function can't be treat as normal call because

To be valid stateless function, we have to check that the return type match JSXElementType (see :defaultTryGetJsxStatelessFunctionAttributesType) I don't want to move that check here since it is such JSX specific.

When trying to resolve JSX tag-name that is not intrinsic, we do following : try as if it is stateless, if fails then try as stateful. #Resolved

sandersn · 2016-11-09T21:44:15Z

src/compiler/checker.ts

+         * @return a resolved signature if we can find function matching function signature through resolve call or a first signature in the list of functions.
+         *         otherwise return undefined if tag-name of the opening-like element doesn't have call signatures
+         */
+        function resolvedStateLessJsxOpeningLikeElement(openingLikeElement: JsxOpeningLikeElement, elementType: Type, candidatesOutArray: Signature[]): Signature {


More MicroSoft disease: StateLess → Stateless #Closed

Also I think resolved should be resolve so that the first word is a verb #Closed

sandersn · 2016-11-09T21:44:38Z

src/compiler/checker.ts

+         * @param elementType
+         * @param candidatesOutArray
+         */
+        function getResolvedJSXStatelessFunctionSignature(openingLikeElement: JsxOpeningLikeElement, elementType: Type, candidatesOutArray: Signature[]): Signature {


JSX → Jsx #Closed

sandersn · 2016-11-09T21:47:27Z

src/compiler/checker.ts

+            if (elementType.flags & TypeFlags.Union) {
+                const types = (elementType as UnionType).types;
+                let result: Signature;
+                types.map(type => {


should just be for (const type of types) ? #Closed

sandersn · 2016-11-09T22:17:41Z

src/compiler/checker.ts

+        function getResolvedJSXStatelessFunctionSignature(openingLikeElement: JsxOpeningLikeElement, elementType: Type, candidatesOutArray: Signature[]): Signature {
+            Debug.assert(!(elementType.flags & TypeFlags.Union));
+            const links = getNodeLinks(openingLikeElement);
+            // If getResolvedSignature has already been called, we will have cached the resolvedSignature.


This code looks a lot like getResolvedSignature. Are you sure you can't re-use getResolvedSignature and move the core difference either down in to resolveStatelessJsxOpeningLikeElement or up into tryGetJsxStatelessFuncitonAttributesType? #Pending

sandersn · 2016-11-09T22:25:41Z

src/compiler/checker.ts

+                let callSignature: Signature;
+                callSignature = resolveCall(openingLikeElement, callSignatures, candidatesOutArray);
+                return callSignature;
+            }


Why do we need to fill up candidatesArray? Is it for the language service? Shouldn't normal resolveCall fill up candidatesArray for a union? #Closed

the function resolveCall will fill up candidatesArray which is pass in from language service. I don't quite understand the second question XD - what union are you referring to?

In reply to: 87291084 [](ancestors = 87291084)

sandersn

Comments for rest of source. Looking at tests next.

sandersn · 2016-11-09T22:58:04Z

src/harness/harness.ts

@@ -988,6 +994,24 @@ namespace Harness {
                }
            }

+            if (libFiles) {


is this needed as part of this PR or can it be broken into a separate change? #Closed

it kind of because otherwise test will be very slow since we will reparse lib file everytime
#Resolved

sandersn · 2016-11-09T23:37:08Z

src/services/completions.ts

+                            //          attributes: JsxAttributes
+                            //             properties: NodeArray<JsxAttributeLike>
+                            //                  each JsxAttribute can have initializer as JsxExpression
+                            return /*JsxExpression*/parent./*JsxAttribute*/parent./*JsxAttributes*/parent.parent as JsxOpeningLikeElement;


the inline comments are a neat idea, but I think they're still too distracting for their value. It's probably good enough to have the tree comment preceding the line of code #Closed

sandersn · 2016-11-09T23:37:32Z

src/services/completions.ts

-                            return <JsxOpeningLikeElement>parent.parent.parent;
+                            parent.parent && parent.parent.kind === SyntaxKind.JsxAttribute) {
+                            // Currently we parse JsxOpeninLikeElement as:
+                            //      JsxOpeninLikeElement


JsxOpeningLikeElement is missing a 'g', everywhere in the comments of this file. #Closed

sandersn · 2016-11-09T23:39:28Z

src/services/findAllReferences.ts

                    return forEach(typeChecker.getRootSymbols(contextualSymbol), s => searchSymbols.indexOf(s) >= 0 ? s : undefined);
                });

                if (contextualSymbol) {
                    return contextualSymbol;
                }

-                // If the reference location is the name of property from object literal destructuring pattern
+                // If the reference location is the name of property from object literal destructing pattern


original spelling was correct #Closed

sandersn · 2016-11-09T23:47:04Z

src/services/goToDefinition.ts

+            //      /*firstSource*/declare function MainButton(buttonProps: ButtonProps): JSX.Element;
+            //      /*secondSource*/declare function MainButton(linkProps: LinkProps): JSX.Element;
+            //      /*thirdSource*/declare function MainButton(props: ButtonProps | LinkProps): JSX.Element;
+            //      let opt = <Main/*firstTarget*/Button />;  // We get undefined for resolved signature indicating an error, then just return the first declaration


wouldn't it make more sense to return the last one since it's the most general? #Pending

hmmm I was trying to mimic how normal call-expression does when there is an error. I swear I thought it goto the first declaration but I was wrong. I think then it should go to the last one

In reply to: 87302820 [](ancestors = 87302820)

did you already change this to returning the last? I thought I saw the change but now I can't find it.

In reply to: 88111830 [](ancestors = 88111830,87302820)

The change is done in checker resolveCall by returning "candidateForArgumentError" instead of undefined. Though I should remove this.

In reply to: 88706239 [](ancestors = 88706239,88111830,87302820)

sandersn · 2016-11-09T23:50:40Z

src/services/goToDefinition.ts

+            if (contextualType) {
+                let result: DefinitionInfo[] = [];
+                forEach(getPropertySymbolsFromContextualType(typeChecker, container), contextualSymbol => {
+                    result = result.concat(getDefinitionFromSymbol(typeChecker, contextualSymbol, node));


how does this differ from for( ... of ... ) #Closed

It doesn't, as far as I can tell, so I would use for-of.

Doesn't this loop get every property of the contextual type, instead of just the one that matches /*1*/? #Closed

I am not sure I understand "just the one that matches but if i understand correctly, the function getPropertySymbolsFromContextualType done the filtering already

In reply to: 87305445 [](ancestors = 87305445)

I was wrong -- getPropertySymbolsForContextualType could return undefined if the property it finds has no name, which it might not for some computed properties. (Probably should have a test for this case too)

In reply to: 87303918 [](ancestors = 87303918)

Yeah, the older version was better. Sorry for the bad suggestion.

In reply to: 88699276 [](ancestors = 88699276,87303918)

sandersn · 2016-11-10T00:14:46Z

src/services/services.ts

@@ -1967,6 +1967,76 @@ namespace ts {
        }
    }

+    function isObjectLiteralPropertyDeclaration(node: Node): node is ObjectLiteralElement  {


shouldn't it be ObjectLiteralElementLike? I didn't think JsxAttribute was part of the main ObjectLiteralElement union.

Does this function exist elsewhere? I thought the rest of the compiler might need this too. But maybe it doesn't. #Closed

it shouldn't be just objectLiteralElementLike because that only includes "PropertyAssignment | ShorthandPropertyAssignment | MethodDeclaration | AccessorDeclaration | SpreadAssignment;" It should just be ObjectLiteralElement as ObjectlIteralElement is the base interface for ObjectLiteralElementLike and JsxAttributesLike. We want to include Jsx here so that when we goto-definition or find all reference we actually look into contextual type flow from stateless function

In reply to: 87306417 [](ancestors = 87306417)

OK, I got the names backward then.

In reply to: 88125144 [](ancestors = 88125144,87306417)

sandersn · 2016-11-10T00:26:06Z

src/services/signatureHelp.ts

@@ -295,7 +296,7 @@ namespace ts.SignatureHelp {

            // findListItemInfo can return undefined if we are not in parent's argument list
            // or type argument list. This includes cases where the cursor is:
-            //   - To the right of the closing paren, non-substitution template, or template tail.
+            //   - To the right of the closing parenthesize, non-substitution template, or template tail.


parenthesis #Closed

sandersn · 2016-11-10T00:29:48Z

src/services/signatureHelp.ts

+            // This is not guarantee that JSX tag-name is resolved into stateless function component. (that is done in "getSignatureHelpItems")
+            // i.e
+            //      export function MainButton(props: ButtonProps, context: any): JSX.Element { ... }
+            //      <MainBuntton /*signatureHelp*/


typo:MainButton #Closed

sandersn · 2016-11-10T17:09:36Z

src/services/symbolDisplay.ts

                    if (!signature && candidateSignatures.length) {
                        // Use the first candidate:
                        signature = candidateSignatures[0];
                    }

-                    const useConstructSignatures = callExpression.kind === SyntaxKind.NewExpression || callExpression.expression.kind === SyntaxKind.SuperKeyword;


I liked this form better. You just have to do more formatting now that the predicate is longer. #Closed

…alType property directly

Address comment: using ternary operator, fix comments

…AttributesTypeFromAttributesProperty

…ement` from inside `checkApplicableSignature`

…eralPropertyDeclaration => isObjectLiteralElement

… error in statelss function component

sandersn · 2016-11-18T16:38:41Z

src/compiler/checker.ts

+         */
+        function checkJsxAttributesAssignableToTagnameAttributes(openingLikeElement: JsxOpeningLikeElement) {
+            // The function involves following steps:
+            //      1. Figure out expected attributes type expected by resolving tag-name of the JSX opening-like element, tagetAttributesType.


typo:targetAttributesType

sandersn · 2016-11-18T16:41:57Z

src/compiler/checker.ts

+            attributesTable = createMap<Symbol>();
+            if (attributesArray) {
+                forEach(attributesArray, (attr) => {
+                    if (!filter || (filter && filter(attr))) {


filter [](start = 36, length = 6)

this part of the predicate is implied by the preceding !filter || so you can simplify to !filter || filter(attr)

sandersn · 2016-11-18T16:44:07Z

src/compiler/checker.ts

+            // i.e <div attr1={10} attr2="string" />
+            //     attr1 and attr2 are treated as JSXAttributes attached in the JsxOpeningLikeElement as "attributes". They resolved to be sourceAttributesType.
+            const sourceAttributesType = createJsxAttributesTypeFromAttributesProperty(openingLikeElement,
+                (attribute: Symbol) => {


(attribute: Symbol) => [](start = 16, length = 22)

can't you just say attribute => ? Or is there some reason to state the type here?

sandersn · 2016-11-18T17:09:08Z

src/services/goToDefinition.ts

+        // object literal, lookup the property symbol in the contextual type, and use this for goto-definition.
+        // For example
+        //      interface Props{
+        //          /first*/prop1: number


/first*/ [](start = 20, length = 8)

should be /*first*/

sandersn · 2016-11-18T17:14:20Z

src/services/services.ts

+            const result: Symbol[] = [];
+            const symbol = contextualType.getProperty(name);
+            if (symbol) {
+                result.push(symbol);


why not return [symbol] here? For a union, do you want both the union's synthetic property and all constituent properties? Seems like the union check should come first and it should return result at the end too.

sandersn · 2016-11-18T17:16:09Z

src/services/goToDefinition.ts

+        //      Foo( { pr/*1*/op1: 10, prop2: true })
+        const container = getContainingObjectLiteralElement(node);
+        if (container) {
+            const contextualType = typeChecker.getContextualType(node.parent.parent as Expression);


node.parent.parent [](start = 65, length = 18)

why not container.parent here? They are the same thing. Also, contextualType isn't used later -- it's just checked. You could inline this variable, but it's probably better to have propertySymbols return undefined if it doesn't find a contextual type.

sandersn · 2016-11-18T17:18:09Z

src/services/services.ts

+        return false;
+    }
+
+    function getNameFromObjectLiteralElement(node: ObjectLiteralElement) {


getNameFromObjectLiteralElement [](start = 13, length = 31)

I think getTextOfPropertyName from compiler's utilities is exactly the same as this code.

Well, getTextOfPropertyName(node.name) ===getNameFromObjectLiteralElement(node)`

In reply to: 88698797 [](ancestors = 88698797)

sandersn · 2016-11-18T18:01:43Z

src/services/goToDefinition.ts

+        //      }
+        //      function Foo(arg: Props) {}
+        //      Foo( { pr/*1*/op1: 10, prop2: true })
+        const container = getContainingObjectLiteralElement(node);


container [](start = 14, length = 9)

can you name container to something more meaningful like element?

sandersn

I like the changes although I still have some questions about gotoDefinition.

Kanchalai Tanglertsampan added 15 commits November 7, 2016 17:01

Parse JSX attributes as its own unique AST node and bind the node

7d02df8

Use chooseOverload logic to pick JSX stateless function

2eeabeb

Update emitter to use JSXAttributes node instead of JSXAttribute node…

e0a5d63

… array

Add language service support for JSXAttributes

e5c9409

Add language service support for JSXAttributes Add completion support Add find-all-references support Add goto-definition support

Update existed conformance tests

fdc6d74

Add tests for using spread attributes resolution in JSX stateful comp…

b71f142

…onent

Add tests for using default attributes in JSX stateful component

4dfe8e8

Add tests for overload stateless function component

4f221d9

Add tests for generic stateless function component

3bd435f

Add a test for contextual type in JSXAttributes

d73d2b8

Update react.d.ts used during tests

c86290c

Cache react.d.ts library when running tests

e7ae8c0

Update conformance tests baselines

bb4227f

Add language service tests

03a57b2

Fix linting error

d3aaf30

msftclas added the cla-not-required label Nov 8, 2016

sandersn requested changes Nov 9, 2016

View reviewed changes

sandersn requested changes Nov 10, 2016

View reviewed changes

Kanchalai Tanglertsampan added 9 commits November 14, 2016 11:19

Address comment: call getContextualType instead of accessing contextu…

13ca860

…alType property directly

Address comment: stop widen type when checking for spread any;

7a95f39

Address comment: using ternary operator, fix comments

Refactor getJsxAttributeSymbolsFromJsxOpeningLikeElement to createJsx…

eff392e

…AttributesTypeFromAttributesProperty

Address comment: fix spelling mistakes

83992ea

Address comment: calling `checkApplicableSignatureForJsxOpeningLikeEl…

b9482f8

…ement` from inside `checkApplicableSignature`

Address comment: fix spelling, rename function to be more consistent

075a8ce

Address comment: minor fix indentation, fix function name isObjectLit…

c1c1e07

…eralPropertyDeclaration => isObjectLiteralElement

Address PR: gotoDefinition return the last signature when there is an…

2a62f49

… error in statelss function component

Address PR: convert Foreach to for...of

46b8f32

sandersn reviewed Nov 18, 2016

View reviewed changes

sandersn requested changes Nov 18, 2016

View reviewed changes

Address comment: fix type, inline code, clarify name of variables

6e91aee

mhegazy mentioned this pull request Dec 30, 2016

[WIP] Using overload for JSX stateless function component #11871

Closed

3 tasks

This was referenced Jan 17, 2017

JSX element attributes type […] may not be a union type #13526

Closed

Using lookahead to detect generic JSX elements? #6395

Closed

yuit mentioned this pull request Jan 23, 2017

[Wip] master stateless overload #13640

Merged

yuit closed this Jan 26, 2017

yuit deleted the wip-statelessOverload2 branch May 9, 2017 16:04

microsoft locked and limited conversation to collaborators Jun 19, 2018

[WIP] Clean up version of using overload for JSX stateless function component #12107

[WIP] Clean up version of using overload for JSX stateless function component #12107

Conversation

yuit commented Nov 8, 2016

sandersn left a comment

Choose a reason for hiding this comment

sandersn Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 8, 2016 • edited by yuit Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

yuit Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited by yuit Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited by yuit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

yuit Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn Nov 10, 2016 • edited by yuit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

sandersn Nov 8, 2016 •

edited

Loading

sandersn Nov 8, 2016 •

edited

Loading

sandersn Nov 8, 2016 •

edited

Loading

sandersn Nov 8, 2016 •

edited by yuit

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

yuit Nov 15, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited by yuit

Loading

sandersn Nov 9, 2016 •

edited by yuit

Loading

sandersn Nov 9, 2016 •

edited

Loading

yuit Nov 15, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 9, 2016 •

edited

Loading

sandersn Nov 10, 2016 •

edited

Loading

sandersn Nov 10, 2016 •

edited

Loading

sandersn Nov 10, 2016 •

edited

Loading

sandersn Nov 10, 2016 •

edited

Loading

sandersn Nov 10, 2016 •

edited by yuit

Loading

sandersn Nov 18, 2016 •

edited

Loading