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

JSXIdentifierReference type #3528

Closed
overlookmotel opened this issue Jun 4, 2024 · 4 comments · Fixed by #5223
Closed

JSXIdentifierReference type #3528

overlookmotel opened this issue Jun 4, 2024 · 4 comments · Fixed by #5223
Assignees
Labels
A-ast Area - AST good first issue Experience Level - Good for newcomers

Comments

@overlookmotel
Copy link
Contributor

Foo in <Foo /> is equivalent to an IdentifierReference. It should have a reference_id, and semantic should resolve its binding.

JSXIdentifier is also used in other positions e.g. as a JSXAttributeName, so would probably need a separate type JSXIdentifierReference for this specific use.

Could also differentiate between <div /> and <Div /> in parser, with JSXIdentifierReference only used for the latter.

@overlookmotel overlookmotel added the C-bug Category - Bug label Jun 4, 2024
@overlookmotel overlookmotel changed the title Add reference_id field to JSXIdentifier in some positions JSXIdentifierReference type Jun 4, 2024
@Boshen
Copy link
Member

Boshen commented Jun 4, 2024

I confirmed with tsc, which uses a plain Identifier:

export type JsxTagNameExpression =
    | Identifier
    | ThisExpression
    | JsxTagNamePropertyAccess
    | JsxNamespacedName
    ;

which means we can remove JsxIdentifier and use IdentifierReference and IdentifierName directly.

pub enum JSXElementName<'a> {
    /// `<Apple />`
    IdentifierReference(Box<'a, IdentifierReference<'a>>),
    /// `<div />
    IdentifierName(Box<'a, IdentifierName<'a>>),
    /// `<Apple:Orange />`
    NamespacedName(Box<'a, JSXNamespacedName<'a>>),
    /// `<Apple.Orange />`
    MemberExpression(Box<'a, JSXMemberExpression<'a>>),
}

but, in order to make it compatible with estree, we need to add at least one new AST node JSXIdentifierReference

:-(

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Jun 5, 2024

Ditto for:

pub enum JSXMemberExpressionObject<'a> {
    /// `<Apple.Orange />`
    IdentifierReference(Box<'a, IdentifierReference<'a>>),
    /// `<div.weird />`
    IdentifierName(Box<'a, IdentifierName<'a>>),
    /// `<Apple.Orange.Banana />` / `<div.weird.bizarre />`
    MemberExpression(Box<'a, JSXMemberExpression<'a>>),
}

I think we can use IdentifierReference and IdentifierName directly and get ESTree compatibility with a custom Serialize impl.

It's unclear to me how namespaces work. Is Apple in <Apple:Orange /> an IdentifierReference or not? React does not support JSX namespaces, so I think we can ignore this question and leave it as is.

@overlookmotel
Copy link
Contributor Author

Looks like JSXElementName also needs a ThisExpression variant. <this /> is valid syntax.

@overlookmotel
Copy link
Contributor Author

Dunqing did bulk of this work in #5223.

Remaining bits to cover: #5352, #5353, #5354.

Closing in favour of those individual issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST good first issue Experience Level - Good for newcomers
Projects
None yet
3 participants