Restructure ast to parse react attributes as object literal#11114
Conversation
…utes to extend from. Create ObjectLiteralElementLike and make ObjectLiteralElement become union type of all possible type allow in PropertyDefinition
…ObjectLiteralExpression.
| // Property assignments | ||
| PropertyAssignment, | ||
| ShorthandPropertyAssignment, | ||
| SpreadObjectLiteralAssignment, |
There was a problem hiding this comment.
There's already a SpreadElementExpression used for array spread elements - is it possible we can use that?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think we still need this kind, but I think SpreadElement is a better name.
There was a problem hiding this comment.
Actually, I'll swap the name of the existing SpreadElementExpression with this in my PR, and you can just drop this change for now.
| expression: Expression; | ||
| } | ||
|
|
||
| // The reason we create this interface so that JSXAttributes and ObjectLiteralExpression interface can extend out of it. |
| } | ||
|
|
||
| // 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 |
There was a problem hiding this comment.
differs -> differ
Add an 's' to the end of ObjectLiteralExpression, JSXAttribute, JSXSpreadAttribute, etc.
What do you mean by "take"?
There was a problem hiding this comment.
JSXAttributes is like objectLiteralExpression but its properties can only be JSXAttribute or JSXSpreadAttribute. I will clarify
sandersn
left a comment
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /** | ||
| * This interface is a base interface for ObjectLiteralExpression and JSXAttributes to extend from. JSXAttributes is similar to |
There was a problem hiding this comment.
I guess JSXAttributes isn't in this change?
| * 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| // @kind(SyntaxKind.SpreadObjectLiteralAssignment) | ||
| export interface SpreadObjectLiteralAssignment extends ObjectLiteralElementLike, SpreadElementExpression { | ||
| _spreadObjectLiteralAssignmentBrand: any; | ||
| dotDotDotToken?: Node; |
There was a problem hiding this comment.
I though you will need in your spread-operator. I was wrong -> remove
| name?: PropertyName; | ||
| } | ||
|
|
||
| export type ObjectLiteralElement = PropertyAssignment | ShorthandPropertyAssignment | SpreadObjectLiteralAssignment | MethodDeclaration | AccessorDeclaration; |
There was a problem hiding this comment.
I don't understand the addition of ObjectLiteralElementLike plus this type. Why is it split now?
There was a problem hiding this comment.
From our in-person discussion, this is to capture JSX-based types later.
Also, we decided to swap the -Like/no-Like names.
| // Property assignments | ||
| PropertyAssignment, | ||
| ShorthandPropertyAssignment, | ||
| SpreadObjectLiteralAssignment, |
There was a problem hiding this comment.
I think we still need this kind, but I think SpreadElement is a better name.
| // Property assignments | ||
| PropertyAssignment, | ||
| ShorthandPropertyAssignment, | ||
| SpreadObjectLiteralAssignment, |
There was a problem hiding this comment.
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
@sandersn this is my restructure of the AST hierarchy to support spread in JSXAttributes essentially JsxAttributes will take the following form