Skip to content
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

JsxTagNameExpression can only be Identifier | ThisExpression, not any PrimaryExpression #21555

Merged
7 commits merged into from
Jun 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 1, 2018

Noticed this while looking at some jsx-using code -- we were typing this as PrimaryExpression but could be a lot more strict.

@ghost ghost requested review from rbuckton and uniqueiniquity February 1, 2018 22:53
@uniqueiniquity
Copy link
Contributor

uniqueiniquity commented Feb 1, 2018

I mean, while you're at it, it seems like PropertyAccessExpression is not particularly strict either, given JSXMemberExpression in https://github.com/facebook/jsx/blob/master/README.md. I think your change is fine but I would recommend waiting for someone more familiar with our grammar to agree.

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

This is fine, but as @uniqueiniquity mentioned, if we're that concerned about strictness here then we would probably want to add a:

export type JsxTagNameExpression = Identifier | ThisExpression | JsxMemberExpression;
export interface JsxMemberExpression extends PropertyAccessExpression {
  expression: JsxTagNameExpression;
}

@ghost
Copy link
Author

ghost commented Feb 2, 2018

I think we could just use EntityNameExpression | ThisExpression, but let's wait on #21588 first.

@ghost
Copy link
Author

ghost commented Feb 5, 2018

@uniqueiniquity @rbuckton Please re-review

@ghost ghost force-pushed the jsxtagnameexpression branch from 909cc64 to a441800 Compare February 5, 2018 22:29
@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@ghost ghost reopened this Jun 14, 2018
@ghost
Copy link
Author

ghost commented Jun 14, 2018

@uniqueiniquity @rbuckton Please re-review

@ghost ghost merged commit c5bd040 into master Jun 29, 2018
@ghost ghost deleted the jsxtagnameexpression branch June 29, 2018 23:50
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants