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

Add ThisExpression variants to JSXElementName and JSXMemberExpressionObject #5352

Closed
overlookmotel opened this issue Aug 30, 2024 · 2 comments · Fixed by #5466
Closed

Add ThisExpression variants to JSXElementName and JSXMemberExpressionObject #5352

overlookmotel opened this issue Aug 30, 2024 · 2 comments · Fixed by #5466
Assignees
Labels
A-ast Area - AST

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 30, 2024

#5223 did the bulk of the work of revamping JSXElementName (#3528). Only thing that's missing is to add:

  • JSXElementName::ThisExpression
  • JSXMemberExpressionObject::ThisExpression

A couple of notes:

Arrow functions transform

This code is currently broken:

let ident = match name {
JSXElementName::Identifier(ident) => ident,
JSXElementName::MemberExpression(member_expr) => {
member_expr.get_object_identifier_mut()
}
JSXElementName::IdentifierReference(_) | JSXElementName::NamespacedName(_) => return,
};
if ident.name == "this" {
// We can't produce a proper identifier with a `ReferenceId` because `JSXIdentifier`
// lacks that field. https://github.com/oxc-project/oxc/issues/3528
// So generate a reference and just use its name.
// If JSX transform is enabled, that transform runs before this and will have converted
// this to a proper `ThisExpression`, and this visitor won't run.
// So only a problem if JSX transform is disabled.
let new_ident = self.get_this_name(ctx).create_read_reference(ctx);
ident.name = new_ident.name;
}

We can fix after we have the ThisExpression variants.

Linter

Once this is done, we can remove these lines from linter.

let name = ident.name.as_str();
// TODO: Remove this check once we have `JSXMemberExpressionObject::ThisExpression`
if name == "this" {
return;
}

@Dunqing
Copy link
Member

Dunqing commented Aug 30, 2024

Before doing this, we can do some preparation work here

/// Change <this></this> to <_this></_this>, and mark it as found
fn enter_jsx_element_name(&mut self, name: &mut JSXElementName<'a>, ctx: &mut TraverseCtx<'a>) {
if !self.is_inside_arrow_function() {
return;
}
if let JSXElementName::Identifier(ident) = name {
if ident.name == "this" {
// We can't produce a proper identifier with a `ReferenceId` because `JSXIdentifier`
// lacks that field. https://github.com/oxc-project/oxc/issues/5352
// So generate a reference and just use its name.
// If JSX transform is enabled, that transform runs before this and will have converted
// this to a proper `ThisExpression`, and this visitor won't run.
// So only a problem if JSX transform is disabled.
let new_ident = self.get_this_name(ctx).create_read_reference(ctx);
ident.name = new_ident.name;
}
}
}
/// Change <this.foo></this.foo> to <_this.foo></_this.foo>, and mark it as found
fn enter_jsx_member_expression_object(
&mut self,
node: &mut JSXMemberExpressionObject<'a>,
ctx: &mut TraverseCtx<'a>,
) {
if !self.is_inside_arrow_function() {
return;
}
if let JSXMemberExpressionObject::IdentifierReference(ident) = node {
if ident.name == "this" {
let new_ident = self.get_this_name(ctx).create_read_reference(ctx);
ident.name = new_ident.name;
}
}
}

We should mutate node instead of mutating the node property. Because after JSXElementName::ThisExpression was added, we need to transform JSXElementName::ThisExpression to JSXElementName::IdentifierReference so that we must mutate the node itself.

@overlookmotel
Copy link
Contributor Author

I think we were having the same thought simultaneously! #5356 (review)

@overlookmotel overlookmotel self-assigned this Sep 4, 2024
Boshen pushed a commit that referenced this issue Sep 5, 2024
…XMemberExpressionObject` (#5466)

Close #5352.

Add to AST:

* `JSXElementName::ThisExpression` (`<this>`)
* `JSXMemberExpressionObject::ThisExpression` (`<this.foo>`, `<this.foo.bar>`)
@Dunqing Dunqing closed this as completed Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment