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

Added ChainExpression type #399

Merged
merged 17 commits into from
Sep 12, 2020
Merged

Added ChainExpression type #399

merged 17 commits into from
Sep 12, 2020

Conversation

ethancrook99
Copy link
Contributor

@ethancrook99 ethancrook99 commented Aug 25, 2020

Added a new ChainExpression type to line up with how [email protected] handles optional chaining. I left OptionalChainExpression but marked it as deprecated for projects that use older babel versions.

Meant to fix this issue

@ethancrook99
Copy link
Contributor Author

@benjamn Can I get a review on this?

Copy link
Owner

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

@ethancrook99 Thanks for getting this started!

Can you try to match the subtyping relationships for this type in the ESTree specification? I think ChainExpression should be an Expression rather than a MemberExpression, and have an expression field that's a ChainElement, which is the supertype of both MemberExpression and CallExpression.

The tools provided by ast-types should be able to capture those relationships, but feel free to ask questions if anything seems tricky.

@ethancrook99
Copy link
Contributor Author

@benjamn Sure, I'll get started on those changes!

@ethancrook99
Copy link
Contributor Author

ethancrook99 commented Aug 26, 2020

@benjamn I changed the definition for ChainExpression, and also added a ChainElement type. I wasn't sure what to do about this though--

extend interface CallExpression <: ChainElement {}
extend interface MemberExpression <: ChainElement {}

Do any changes need to be made to CallExpression and MemberExpression to make this work correctly?

@ethancrook99
Copy link
Contributor Author

ethancrook99 commented Aug 31, 2020

@benjamn I changed the supertype of CallExpression and MemberExpression to ChainElement, but it's causing a lot of tests to fail because those two types are no longer a subtype of Expression and aren't related to ExpressionKind. Do you know of a way to resolve that conflict? I pushed the changes so you can take a look

@info-bit
Copy link

Are you planning to merge this fix anytime soon? We're stuck with a yarn resolution for babel in the package to support react-docgen.

Copy link
Owner

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

@ethancrook99 Thanks for continuing to work on this, and sorry for leaving you to figure out the key insight about multiple inheritance. That's one of the reasons I built this whole def system in the first place, instead of using ordinary JS prototypal inheritance, since prototypes allow for only single inheritance (with good reason, of course, but you really want multiple inheritance, or something like traits, for an AST type system like this).

Types can be built up incrementally, so we can avoid including the
ChainElement functionality in def/core.
We should never see nor need to create an AST node with type
"ChainElement", so this abstract type doesn't need to be buildable (or
printable by Recast).
Since CallExpression and MemberExpression inherit from this type, and
those types are not optional by default, the default value of the
'optional' field must be false.

As a side benefit, I think this default gives more value to the (otherwise
somewhat redundant) OptionalMemberExpression and OptionalCallExpression
types, whose 'optional' fields are true by default, in contrast to the
ordinary MemberExpression and CallExpression types.
@ethancrook99
Copy link
Contributor Author

Thanks for finishing that up and getting it merged!

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.

Support ChainExpression type for optional chaining
3 participants