Skip to content

AstVisitor corrections #239

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

Merged
merged 4 commits into from
Apr 25, 2022
Merged

AstVisitor corrections #239

merged 4 commits into from
Apr 25, 2022

Conversation

adams85
Copy link
Collaborator

@adams85 adams85 commented Apr 24, 2022

I've found two minor issues related to AstVisitor in the recent commits:

  1. VisitExportAllDeclaration was not updated by Support ES2020 export ns from #196 to include the visitation of the new ExportAllDeclaration.Exported property.
  2. Addition of VisitStaticBlock is rather questionable because it adds redundancy (at least, from the visitor's POV). StaticBlock is just a special case of BlockStatement, has no additional attributes. In such cases we (usually) haven't introduced dedicated visit methods, just left it to the base class to handle the visitation aspect. For example, in the cases of ExpressionStatement and Directive or MemberExpression and StaticMemberExpression. I think it'd be a good idea to keep up this convention.

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Looks good.

@lahma lahma enabled auto-merge (squash) April 25, 2022 06:07
@lahma lahma merged commit 59c1b11 into sebastienros:main Apr 25, 2022
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.

2 participants