Restructure ast to parse react attributes as object literal #11114

yuit · 2016-09-23T23:06:51Z

@sandersn this is my restructure of the AST hierarchy to support spread in JSXAttributes essentially JsxAttributes will take the following form

export interface JsxAttributes extends ObjectLiteralExpressionBase<JsxAttributeLike> {
        _jsxAttributesBrand: any;
}

…utes to extend from. Create ObjectLiteralElementLike and make ObjectLiteralElement become union type of all possible type allow in PropertyDefinition

…ObjectLiteralExpression.

DanielRosenwasser · 2016-09-24T04:53:38Z

src/compiler/types.ts

@@ -316,6 +316,7 @@ namespace ts {
        // Property assignments
        PropertyAssignment,
        ShorthandPropertyAssignment,
+        SpreadObjectLiteralAssignment,


There's already a SpreadElementExpression used for array spread elements - is it possible we can use that?

I thought about it. but i think it is may be more confused to do., @sandersn is putting up PR for spread operator, will see how he handle it over there

I think we still need this kind, but I think SpreadElement is a better name.

Actually, I'll swap the name of the existing SpreadElementExpression with this in my PR, and you can just drop this change for now.

DanielRosenwasser · 2016-09-24T04:54:08Z

src/compiler/types.ts

@@ -1048,10 +1057,16 @@ namespace ts {
        expression: Expression;
    }

+    // The reason we create this interface so that JSXAttributes and ObjectLiteralExpression interface can extend out of it.


DanielRosenwasser · 2016-09-24T04:55:39Z

src/compiler/types.ts

@@ -1048,10 +1057,16 @@ namespace ts {
        expression: Expression;
    }

+    // The reason we create this interface so that JSXAttributes and ObjectLiteralExpression interface can extend out of it.
+    // JSXAttributes differs from normal ObjectLiteralExpression in that JSXAttributes can only take JSXAttribute or JSXSpreadAttribute


differs -> differ
Add an 's' to the end of ObjectLiteralExpression, JSXAttribute, JSXSpreadAttribute, etc.

What do you mean by "take"?

JSXAttributes is like objectLiteralExpression but its properties can only be JSXAttribute or JSXSpreadAttribute. I will clarify

sandersn

Looks mostly good but (1) I don't understand the new type ObjectLiteralElementLike and (2) I need to think about Daniel's comment on SpreadElementExpression.

sandersn · 2016-09-26T17:34:01Z

src/compiler/types.ts

@@ -1048,10 +1057,19 @@ namespace ts {
        expression: Expression;
    }

+    /**
+      * This interface is a base interface for ObjectLiteralExpression and JSXAttributes to extend from. JSXAttributes is similar to


I guess JSXAttributes isn't in this change?

sandersn · 2016-09-26T17:41:25Z

src/compiler/types.ts

+      * JSXAttribute or JSXSpreadAttribute. ObjectLiteralExpression, on the other hand, can only have properties of type
+      * ObjectLiteralElement (e.g. PropertyAssignment, ShorthandPropertyAssignment etc.)
+     **/
+    export interface ObjectLiteralExpressionBase<T extends ObjectLiteralElementLike> extends PrimaryExpression, Declaration {


this name seems weird when used with its type parameter: ObjectLiteralExpressionBase<ObjectLiteralElement>. I would prefer a more generic name instead of the suffix Base, maybe LiteralExpression? LiteralExpression<ObjectLiteralElement> and LiteralExpression<JSXAttribute | JSXSpreadAttribute> looks more reasonable.

As we talked off-line, the name LiteralExpression can be confused with stringLiteral or numericLiteral. I will keep this name for now and if we decide to change, it can be done pretty easily

sandersn · 2016-09-26T17:43:22Z

src/compiler/types.ts

+    // @kind(SyntaxKind.SpreadObjectLiteralAssignment)
+    export interface SpreadObjectLiteralAssignment extends ObjectLiteralElementLike, SpreadElementExpression {
+        _spreadObjectLiteralAssignmentBrand: any;
+        dotDotDotToken?: Node;


why is this optional?

I though you will need in your spread-operator. I was wrong -> remove

sandersn · 2016-09-26T17:45:56Z

src/compiler/types.ts

        _objectLiteralBrandBrand: any;
        name?: PropertyName;
   }

+    export type ObjectLiteralElement = PropertyAssignment | ShorthandPropertyAssignment | SpreadObjectLiteralAssignment | MethodDeclaration | AccessorDeclaration;


I don't understand the addition of ObjectLiteralElementLike plus this type. Why is it split now?

From our in-person discussion, this is to capture JSX-based types later.
Also, we decided to swap the -Like/no-Like names.

sandersn · 2016-09-26T18:07:21Z

src/compiler/types.ts

@@ -316,6 +316,7 @@ namespace ts {
        // Property assignments
        PropertyAssignment,
        ShorthandPropertyAssignment,
+        SpreadObjectLiteralAssignment,


I think we still need this kind, but I think SpreadElement is a better name.

sandersn · 2016-09-26T18:37:44Z

src/compiler/types.ts

@@ -316,6 +316,7 @@ namespace ts {
        // Property assignments
        PropertyAssignment,
        ShorthandPropertyAssignment,
+        SpreadObjectLiteralAssignment,


Actually, I'll swap the name of the existing SpreadElementExpression with this in my PR, and you can just drop this change for now.

…itearlElementLike as a type alias

Kanchalai Tanglertsampan added 2 commits September 23, 2016 15:50

Create base interface for ObjectLiteralExpression or future JSXAttrib…

d739c36

…utes to extend from. Create ObjectLiteralElementLike and make ObjectLiteralElement become union type of all possible type allow in PropertyDefinition

Add spreadObjectLiteralAssignment. This is an AST node for spread in …

78c3f17

…ObjectLiteralExpression.

msftclas added the cla-not-required label Sep 23, 2016

yuit assigned sandersn Sep 23, 2016

DanielRosenwasser reviewed Sep 24, 2016

View reviewed changes

DanielRosenwasser requested changes Sep 24, 2016

View reviewed changes

Address PR: Update comment and use JSDoc

e1bfd7f

sandersn requested changes Sep 26, 2016

View reviewed changes

sandersn approved these changes Sep 26, 2016

View reviewed changes

Kanchalai Tanglertsampan added 2 commits September 26, 2016 11:42

Address PR: use ObjectLiteralElement as an interface name and ObjectL…

3c74558

…itearlElementLike as a type alias

Rename isObjectLiteralElement to isobjectLiteralElementLike

625a253

yuit merged commit 3d92117 into master Sep 26, 2016

yuit deleted the restructureASTToParseReactAttributesAsObjectLiteral branch September 26, 2016 22:12

microsoft locked and limited conversation to collaborators Jun 19, 2018

Restructure ast to parse react attributes as object literal #11114

Restructure ast to parse react attributes as object literal #11114

Uh oh!

Conversation

yuit commented Sep 23, 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!

DanielRosenwasser Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

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!

Uh oh!

DanielRosenwasser Sep 24, 2016 •

edited

Loading