-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Parse eventual-send (tildot) syntax #11485
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0906e9f:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/23363/ |
48195db
to
f20ed8b
Compare
Is the difference between |
No, it is not. |
Ok, then we don't need the
Also, since this PR is quite big, can we keep the transform implementation for a separate PR 🙏 |
bb41161
to
a06877a
Compare
Thanks for catching this. I've updated accordingly.
You bet! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this proposal interact with optional chains? In a?.b~.c.d
, are ~.c
and .d
accessed when a
is nullish? Or is it equivalent to (a?.b)~.c.d
? Is there an operator to do ?.
and ~.
at the same time (e.g. a?~.b
)?
I think that this heavily affects the AST design.
Is the difference between
(a~.b).c
anda~.b.c
observable?
I looked at the proposal's README, and realized that my question was incomplete. Since a~.b()
and (a~.b)()
are different (from what I understood, they are a.[[ApplyMethodSend]]('b', [])
and a.[[GetSend]]('b')()
), we would probably need a new EventualMemberCallExpression
node if we go with the current AST design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(EDIT: Duplicated review comment)
I'll have to defer to @erights on these details.
Agreed.
I had been thinking that |
@@ -537,6 +537,14 @@ export type MemberExpression = NodeBase & { | |||
computed: boolean, | |||
}; | |||
|
|||
export type EventualMemberExpression = NodeBase & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates: As @nicolo-ribaudo points out (a~.b)()
is a~.b()
is not equivalent. So adding eventual: boolean
is not enough. I will post an updated suggestion tomorrow. I am still negative on adding new node types.
w.l.o.g. I will take EventualMemberExpression
as an example, likewise EventualCallExpression
.
I am hesitant about the extra node type EventualMemberExpression
and likely-in-the-future EventualOptionalMemberExpression
/ OptionalEventualMemberExpression
given that optional chaining has reached stage 4.
The wavy-dot
proposal introduces infix operators ~.
similar to optional chaining. I understand that it may seem reasonable to add a new node type, which is what we did in OptionalMemberExpression
. Essentially OptionalMemberExpression
is a MemberExpression
. The introduction of the new node type OptionalMemberExpression
is due to 1) the new optional semantic and 2) the short-circuit behaviour. The latter requires that a new node type has to be introduced because we need to know when the base is nullish, we should skip which parts of the whole optional chains.
EventualMemberExpression
is also a MemberExpression
. Similar to OptionalMemberExpression
, it introduces new eventual semantics. Besides that I don't see there are extra behaviours which we should track on the AST level. Therefore I propose to add an eventual: boolean
property to the existing MemberExpression
interface MemberExpression <: Expression, Pattern {
type: "MemberExpression";
object: Expression | Super;
property: Expression;
computed: boolean;
eventual: boolean;
}
Since the eventual semantic is additive, we can also add eventual: boolean
once the proposal supports optional chaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need EventualMemberCallExpression
, which I would rename to EventualSendExpression
. I'll wait on your updated suggestion before I start changing the current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just a note:
.?~
is sensible (only eventual-send if the lhs is not nullish), but .~?
is not sensible (since eventual-send always returns a Promise, it cannot be nullish).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates: As @nicolo-ribaudo points out
(a~.b)()
isa~.b()
is not equivalent. So addingeventual: boolean
is not enough. I will post an updated suggestion tomorrow. I am still negative on adding new node types.
@JLHwung have you had a chance to think about this? I definitely want to implement the PR in the correct way, but am blocked until I get some design advice.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelfig My approach is now based on the ChainingExpression
approach proposed by ESLint team: See https://gist.github.com/JLHwung/6ec08a87e4da88874c50788c37d6fdf4 for detailed examples.
Note that this approach is tremendously different from current babel's approach and we will discuss if we will go for that path in the upcoming babel meetings. Before any decision is made, I would suggest we hold on a bit until more feedback from the AST shape of wavy-dot
.
Updates: also open estree/estree#216 for AST discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the above, I found estree/estree#204 which is long, but seems to be giving some traction to https://gist.github.com/mysticatea/5453e721892b9c5a621e445f5fcdf62f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelfig My approach is now based on the
ChainingExpression
approach proposed by ESLint team: See https://gist.github.com/JLHwung/6ec08a87e4da88874c50788c37d6fdf4 for detailed examples.Note that this approach is tremendously different from current babel's approach and we will discuss if we will go for that path in the upcoming babel meetings. Before any decision is made, I would suggest we hold on a bit until more feedback from the AST shape of
wavy-dot
.
@JLHwung, has this discussion settled? If so, what does it mean for the kind of AST shape I should target?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bf9b640
to
a5bf839
Compare
a5bf839
to
0906e9f
Compare
@nicolo-ribaudo and @JLHwung, I'd like to see this PR move forward again. Would you be able to help me finish the AST design?
Optional chains can short-circuit eventual sends.
They are short-circuited.
Yes, we would like that.
That's correct.
That seems right to me. |
Let's transfer the discussion about composition between optional chain and wavy-dot to the proposal repo. I think we have to hold off a bit until the draft clearly specifies the proposed syntax change to LeftHandSideExpression. Because
as @nicolo-ribaudo mentioned before. |
This implements the parser changes described in #11348 to implement the stage1 TC39 eventual-send syntax proposal described in that issue.
I hope to land these changes soon, to experiment with plugins that use the tildot syntax. When the proposal is more mature, we will make a new PR to merge the appropriate plugins.