-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Enable parsing of |> await
#7458
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7026/ |
Thanks for the CC, @mAAdhaTTah. For everyone else, let me give some background. @mAAdhaTTah and I are working together to develop specifications and Babel plugins for alternative pipeline-operator proposals. The original pipeline operator proposal was blocked by TC39 at Stage 1 in order to address async functions and to align with other proposals (especially partial application's, also perhaps composition's and method extraction’s) into a more coherent approach. Further discussion yielded four competing proposals. @littledan invited @mAAdhaTTah and me to work on specifications and Babel plugins for our favorite proposals. @mAAdhaTTah is working on Pipeline Proposal 1: F-sharp Style Only. I am working on Pipeline Proposal 4: Smart Mix. Our hope is to be able to present both proposals’ specifications and plugins to the next TC39 meeting. From the perspective of Pipeline Proposal 4, I don’t see a problem with this pull request yet. My only two worries would be:
@mAAdhaTTah: As you say, no changes to the original Stage-1 pipeline proposal are normative; they probably won’t be until our revised proposals will be revisited at the next TC39. Do you think it might be possible to keep this fork separate from upstream Babel until @littledan normatively changes the original Stage-1 pipeline proposal? After all, I probably would implement it as my own fork of In other words: Should we keep our respective Pipeline Proposals’ plugins as forks separate from Babel upstream for now, or should we merge the plugins into Babel upstream under different names? [Edit: #7154 is related to this pull request.] |
@js-choi This parser change shouldn't impact whether the current pipeline plugin functions according to the current spec, nor should it impact our ability to write alternative plugins for the various proposals. This change should simply expand what babylon currently considers valid syntax. I think we should keep our proposals separate from babel proper and merge in whichever version gets accepted at TC39. That should make it clear that what's in babel matches the current spec, and everything outside of it is in a state of flux. Your babel plugin can throw its own errors if you want to disallow |
You don't have to make a new plugin for each one if it makes more sense to just make plugin options for each option. |
@hzoo: God suggestion, thanks. @mAAdhaTTah: I don’t know if it’d be a good idea while we’re still developing rapidly on the plugins, but once those are mostly done, merging them into the same existing plugin could make it easier for TC39 to compare them. I think it’ll eventually be a good idea, though eventually some options would be deprecated as the proposal advances. |
@@ -0,0 +1,3 @@ | |||
async function foo() { | |||
return a |> await |> f; |
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'll need tests for a |> await b |> c
and a |> await
(with/without ASI), too.
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.
Added!
We need to parse the next expression if it's not the next step in the pipeline.
This should cover some additional ways `await` can be used in the pipeline.
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.
Almost there!
@@ -0,0 +1,3 @@ | |||
async function foo() { | |||
return a |> await b |> f; |
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.
This should result in a parse 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.
Do we want it to result in a parse error? The current pipeline plugin transforms this behavior. My approach was to not break the current plugin, which matches the current spec.
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.
My understanding is the pipeline operator is F# style proposal, which we follow until another proposal is presented to TC39. So this should error out, but a |> (await b) |> f
shouldn't.
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.
The current spec hasn't made |> await
normative yet: tc39/proposal-pipeline-operator#85 – so it's not exactly the same as the F#-style, which does. I think we're holding on that PR until we settle the F#- v Hack-style debate. My goal was to make it possible to match all 3 (current spec, F#-style, & Hack-style).
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.
On second thought, I think this needs to parse in order to implement the Hack-style pipelining, which still has a RHS for the await
statement.
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 agree that the grammar of these proposals differs from each other, and it's not the kind of relationship where one is a subset of the other. I don't know what the best way to handle this is technically within Babel, but it seems to me like Babylon will eventually need some kind of mode switch between multiple options. I think it'd be valuable to have both @js-choi and @mAAdhaTTah 's alternatives land in Babel to be usable by actual programmers without them having to check out a custom branch.
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.
@littledan @js-choi I think we can proceed initially w/ proposals 2-4 w/o changing babylon if we start with the placeholder as a valid identifier, e.g. using @gilbert's preference for $_
. Once we have that implemented, we then update babylon to parse the #
as an identifier only within a pipeline, and from there, it shouldn't be too difficult to modify the pipeline plugin to provide the placeholder as a configuration option, including using #
(or even ?
) as a placeholder.
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.
@mAAdhaTTah Maybe that's a reasonable way to stage this, to implement it little by little and land incremental patches, but at some point, I think you'll need to update Babylon and give it multiple modes. I don't see the problem with that, though.
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.
This realization might complicate things, since Babylon doesn’t have a plugin system, right? In order for Babel to support both our Babel plugins and to let TC39 compare our proposals, Babylon needs to parse both.
I had been wrong here; I had forgotten that Babylon does have a “plugin system”. It’s just not open to third parties. This pull request modifies the existing pipelineOperator
plugin.
I wonder if it would be better to isolate Proposal 1 and Proposal 4 into separate Babylon plugins, both separate from the current pipelineOperator
plugin. As @littledan says, it depends on how much their grammars have in common. Alternatively, we could both modify the existing pipelineOperator
plugin but require a pipelineMode
option (we could use the babylon.parse
options parameter – can Babylon plugins themselves accept options too?)
As I said, I’m uncertain about what the precedence and associativity of Proposal 1’s |>
is—but the biggest difference between Proposal 1 and 4 are how they handle arrow functions. x |> $ => $ + 1
would be idiomatic with Proposal 1 but it would be an early error for Proposal 4, in which you would write x |> # + 1
instead. This difference is what, above all, makes the two proposals mutually exclusive. (Bare await
in x |> await
is also forbidden by Proposal 4, but Proposal 4 could hypothetically be extended to allow it.)
A hypothetical version of Proposal 1 – one that accepts only Proposal-4-style “simple references” at its |>
RHS – could be forward compatible with Proposal 4. That is, a version of Proposal 1 whose |>
is also right associative and also just looser than ternary conditional – and which allows x |> functionIdentifier
and x |> object.method
but forbids x |> $ => $ + 1
– would be forward compatible with Proposal 4.
But this solution probably would overcomplicate both proposals’ Babylon implementations. It might be cleanest to just keep both proposals separate.
@mAAdhaTTah: As @littledan says, eventually we will need to update Babylon for Proposal 4 anyway. I feel that it might be best to deal with that now, early on.
@littledan, @mAAdhaTTah: So the three simplest options I see are:
-
A. Put both Proposals 1 and 4 into the existing
pipelineOperator
plugin but require apipelineMode
option (either using thebabylon.parse
options parameter or a plugin-specific option).- @mAAdhaTTah would modify this fork and this pull so that his changes to the existing
pipelineOperator
plugin are isolated behind apipelineMode: 'fSharpWithBareAwait'
flag or whatever. - I would fork @mAAdhaTTah’s branch and isolate my changes behind a
pipelineMode: ’smartMix'
flag. - I would eventually pull request @mAAdhaTTah’s branch with a
smartPipelines
plugin. - After that, @mAAdhaTTah’s branch would in turn eventually be pulled into upstream Babylon, either through this pull request or another pull request.
- Our Babel plugins would both use a single Babylon plugin
pipelineOperator
, respectively usingpipelineMode: 'fSharpWithBareAwait'
andpipelineMode: 'smartMix'
. - Issue: Can Babel plugins configure Babylon using specific Babylon options?
- @mAAdhaTTah would modify this fork and this pull so that his changes to the existing
-
B. Modify the
pipelineOperator
plugin to match Proposal 1, including its bare|> await
form. Put Proposal 4 into a separatesmartPipelines
plugin that is mutually exclusive withpipelineOperator
.- @mAAdhaTTah would keep this fork and this pull request the same, which directly modifies the existing
pipelineOperator
plugin. - I would fork upstream Babylon with a
smartPipelines
plugin, which would be mutually exclusive withpipelineOperator
. I would separately pull request upstream Babylon withsmartPipelines
. - Our Babel plugins would respectively depend on Babylon
pipelineOperator
and BabylonsmartPipelines
.
- @mAAdhaTTah would keep this fork and this pull request the same, which directly modifies the existing
-
C. Isolate Proposal 1 and Proposal 4 into two mutually exclusive Babylon plugins
pipelineBareAwait
andsmartPipelines
, both of which are separate from the currentpipelineOperator
plugin.- @mAAdhaTTah would modify this fork and pull request so that his changes are put in another
pipelineBareAwait
plugin. - I would fork upstream Babylon with a
smartPipelines
plugin, which would be mutually exclusive withpipelineBareAwait
. I would separately pull request upstream Babylon withsmartPipelines
. - Our Babel plugins respectively depend on Babylon
pipelineOperator
+pipelineBareAwait
and BabylonpipelineOperator
+smartPipeline
.
- @mAAdhaTTah would modify this fork and pull request so that his changes are put in another
Or maybe there are other options. I’m fine with whatever, as long as I get to independently muck with the parser too.
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.
@js-choi I think you're overthinking it; we should be able to make babylon produce an AST compatible with all of the proposals, and the have the pipeline plugin itself be responsible for throwing errors when it encounters syntax that isn't valid for the mode it's in, e.g. in F#-mode, we'd throw if we found |> await a
, while Hack-mode would throw on |> await
, etc.
This might make babylon more forgiving around the pipeline operator than it should be, but I think that's ok for now, as the goal here is to get this into the wild and get feedback from developers, and we can tighten up babylon's behavior once pipeline is stage 2.
As I mentioned above, if you start with the valid-identifier version, I don't think switching that to #
, once the changes are made in babylon, will be difficult.
@mAAdhaTTah: All right, thanks for the explanation. I’m mostly concerned about compatibility of associativity and precedence, but I’ll not worry about that yet. I’ll see what I can do with just a fork from your branch, once I finish polishing up my explainer + spec. |
@js-choi Great! And if we have AST conflicts, we can revisit this decision. @jridgewell tl;dr: This is the plan, this should land as-is to get that underway. Concur? |
Personally, I'd much rather enable two parser plugins, one to enable F# and one to enable arbitrary expressions. I don't believe the proposals are semantically compatible. My main issue is |
@jridgewell: For what it’s worth, in the smart proposal, I have just about finished the smart mix’s explainer and its Core Proposal’s spec and I’m ready to implement a Babel plugin. I don’t know whether to use @mAAdhaTTah’s Babylon plugin patch or make a new Babylon plugin—though I’ll probably start off by forking @mAAdhaTTah’s branch either way. |
These proposals are getting so complicated now...
I don't understand this. An early Syntax error is a grammar parse error, right?
Isn't |
@jridgewell: Sorry, I’m pretty sleepy right now, and my brain had a short-circuit. I had realized that I made a mistake and quickly edited my comment—apparently too late to be before you read it. Sorry about that.
I haven’t really announced the [near-]finishing of my explainer and proposal yet, but we can talk about it on my proposal’s repository or in the original pipeline-operator repository. I was planning, tomorrow, to link to it on IRC #tc39 and open an issue in the original pipeline-operator repository, after @littledan took a brief look at it.
By “grammatically parse but throw an early Syntax Error”, I meant that the context-free grammar would successfully construct a Parse Node tree but then an early error rule would be triggered during static analysis.
Sorry again for the confusion. I’d be happy to answer any of your questions elsewhere. Let’s talk about your questions on my repository, on the new original-pipeline-repository issue that I’ll open, or perhaps on IRC. |
@jridgewell These are just two different proposals, with two different syntaxes and semantics. Combining them somehow would be a third proposal. I think it'd be best to have a switch to choose between the two in .babelrc, and not try to figure out how to combine them unless we think that's semantics we might want to go to TC39 with. |
I don't think this matters for babylon. As long as |
That's why I'd prefer two separate parser "plugins", essentially a flag to control which semantics we're using inside the
Ok. |
I agree, and we will have a flag in So I don't think we need separate parser plugins or flags in babylon. The above changes should go into the babel plugin. Does that make sense? Would love to get your approval to land this and start working on said babel plugin, unless you want to see those babel plugin changes as part of this PR. What's the best way forward? |
FYI, after some discussion, it appears we are going to need flags for the two different parse modes, as there are spec differences that make sharing the AST unfeasible. This PR will be updated with those changes, and I'll update the title accordingly once those changes are made. |
To expand on that, I'm okay with the AST types being shared I think, I'm just concerned with sharing the Babylon plugin name and thus delegating responsibility for errors from the parser to the transform. From my standpoint it seems easier to have separate Babylon plugin names for the different options so that Babylon can throw errors up front. |
@loganfsmyth FWIW, I was going to discuss that with you, but we're running into AST differences that make that question moot. |
Ah, okay! That works too then :) |
tl;dr version: The current babel plugin doesn't actually match the current spec. Do we want to maintain the current wrong behavior by default or update to match the correct semantics? We're working on implementing the flags required to enable to multiple proposals in babylon (well, I say "we", but it's really been @js-choi :) ). My original thought was to maintain 3 flags:
In investigating how we're handling arrow functions in the F# proposal, I realized that the babel plugin doesn't actually match the current spec. Based on the discussion here: tc39/proposal-pipeline-operator#70 arrow functions in the pipeline without parens should be a SyntaxError in the current spec. But it gets better: as part of the F# proposal, I'd like to include this suggestion: tc39/proposal-pipeline-operator#104 which would bring back unparen'd arrow function but change the way babylon parses arrow functions at the head of the pipeline. Given this, we can do one of two things:
Does that make sense? Anyone have thoughts / preferences for how we should approach this? |
@mAAdhaTTah If it makes sense to you, I think you and @js-choi should feel free to use the |
@littledan Ok we can go with that; should be update the current |
@mAAdhaTTah Do you think I should revert the |
@littledan We can remove await handling from the spec, which would obviate the need for #7154, but the issue I'm describing is actually related to arrow functions in the pipeline. The current plugin parses them even if they don't have parens, which should be a SyntaxError according to the current spec. |
@mAAdhaTTah OK, I'd welcome a PR for the main spec which makes this change, and then it will make sense to do this other change that you are suggesting in Babel as well. If you want to preserve |
@littledan Ok, just to make sure we're on the same page, next steps are:
👍 this comment if that's correct :). |
That plan sounds good to me. It's unfortunate that developer code may break. However, the impact should be more limited, as pipeline was added recently. cc @jridgewell |
I think we'll have to be careful with breaking changes once we get a release candidate out, but I don't mind breaking changes to things before we reach that. That said, I'm really hoping we'll be close to an RC within a couple weeks. We'll see if that actually pans out though. |
@loganfsmyth This is part of what I was getting at in #7558 - given that the proposals are still in flux, and that relatedly, we're likely to need breaking changes as we go, I'm not sure what the semver policy is for babylon's AST or its internal plugins. |
@loganfsmyth: Would work on the new pipeline stuff block Babel 7’s RC, or would the RC go forward if the new pipelines are not yet ready in the next few weeks? |
To be clear, 'few weeks' is extremely vague on my part, so don't use it as a hard number. :) I honestly don't know. Generally by the time Babel has implemented a proposal, it has already stabilized a decent amount so the syntax changes have been minimal. This is a first time for us, and I think honestly it is going to be a little painful because we're just not in a place where frequent AST changes are easily adaptable for Babel. I'm not familiar enough with the pipeline stuff to know, but I guess I had assumed that because the different options were based on existing syntax in other languages that they wouldn't be as aggressively changing a huge amount. |
To expand a bit, the thing that concerns me, WRT the F# Proposal specifically, is I'd like to implement it for now with this suggestion to get feedback: tc39/proposal-pipeline-operator#104 If we decide that this is not a tenable approach and need to require parens around the arrow functions, introducing that change behind the same flag later (let's say post-RC) becomes a breaking change for babylon (and for the pipeline plugin), and its not really clear to me how we should handle that possibility in the future (or now, for that matter) This is why I wanted to reiterate the fact that the changes to conform to the spec is technically breaking, and we feasible could even end up reverting the change again if the F# Proposal gets accepted. I know this sort of experimentation is new for babel, so the policies aren't sorted, but that's the issue we're looking at and would need a policy around. Is there any history, maybe related to how decorators were handled, that could be guiding? |
What is Babel’s semver policy with regard to breaking experimental Babylon plugins? Is such a policy yet set in stone? The reason why we want to use Babylon for mutually exclusive experimental syntaxes is so that TC39 and developers in general can try out competing TC39 proposals, hands on, and give feedback about them. As has been mentioned by others, the same has been occurring for decorators, except now we have two mutually exclusive Proposal 1: F-sharp Style Only proposals, too—F-sharp Style Only with pipelines looser than arrow functions versus F-sharp Style Only with pipelines tighter than arrow functions. And that’s assuming both allow bare await in their pipeline bodies. If the answer is, “Breaking even an experimental plugin is a major-version change,” then would it be better to separate Babylon |
With regard to my last question, about semver with experimental plugins: Looks like relevant discussion is occurring in #7558. |
82e2500
to
a644d6b
Compare
Since this is going to expand quite a bit, we're going to close this PR and reopen it when all 3 flags are implemented. |
As part of the work on the pipeline operator, the suggestion for F# style (Proposal 1) is the enable
|> await
without a RHS expression. It does break any other tests, but it might break|> await expr
, which is currently supported in the pipeline plugin, although those tests don't fail, so maybe not.I think we should support both, as the spec for the pipeline operator hasn't actually been updated to make
|> await
normative (see here). The idea is to enable us to develop babel plugins around the proposals, so I think we should prefer not to break the current babel plugin, since it matches the current spec.This may not be ready, but I'm definitely struggling a bit here, so hoping to get some thoughts / suggestions / feedback on this.
cc @js-choi @littledan