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
Fix right precedence of Hack pipes #13668
Fix right precedence of Hack pipes #13668
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48595/ |
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 b984f7e:
|
Was this intentional, or just an oversight? I think every other operator is left associative (with the exception of |
@jridgewell: Yeah, Hack pipes should be right-associative, not left-associative. Currently, Hack pipes are functionally bidirectionally associative, so In fact, after taking with @nicolo-ribaudo, I plan to loosen the precedence of Hack pipes in the specification to AssignmentExpression, to match Of course, for |
Let's keep this PR only for the associativity, to give some time for the discussion at tc39/proposal-hack-pipes#11. |
This pull request could probably now be updated to the new changes as per tc39/proposal-hack-pipes#11 and tc39/proposal-hack-pipes#12:
|
467cac3
to
9310eec
Compare
Updated to reflect the precedence currently defined in the proposal. |
Thanks for continuing to work on this, @nicolo-ribaudo. Once we merge these in, I can work on support for |
|
||
const body = this.parseMaybeAssign(); | ||
|
||
const invalidBodies = { |
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.
Consider hoist this object to top level so it doesn't have to be re-created in parseHackPipeBody()
.
ArrowFunctionExpression: "arrow function", | ||
AssignmentExpression: "assignment", | ||
ConditionalExpression: "conditional", | ||
YieldExpression: "yield", |
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.
Could leave a todo item here that invalidBodies
currently does not handle TS extensions such as TSAsExpression
, they will be supported after TypeScript supports hack pipes.
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 added a todo, but I don't think that it will be a problem because:
a = b as c
is an AssignmentExpressiona => b as c
is an ArrowFunctionExpressiona ? b : c as d
is a ConditionalExpressionyield as a
is a syntax error
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.
Oh I was meant to the plain as expression a as c
. The precedence of as
and |>
are unclear before TS supports pipes.
YieldExpression: "yield", | ||
}; | ||
|
||
if (hasOwn(invalidBodies, body.type) && !body.extra?.parenthesized) { |
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.
We can transform invalidBodies
to a map so it comes with .has()
, and it's also faster than objects.
Spec: http://jschoi.org/21/es-hack-pipes/#sec-pipe-operator