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

Implement Smart Pipeline proposal in @babel/parser #8289

Merged
merged 27 commits into from Dec 3, 2018

Conversation

mAAdhaTTah
Copy link
Contributor

@mAAdhaTTah mAAdhaTTah commented Jul 8, 2018

Q                       A
Fixed Issues? Related to babel/proposals#29
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes?
License MIT

Most of this work was implemented by the illustrious @js-choi, which some adjustments and refreshments from myself to use the new plugin options API. He's been fairly busy so I'm going to be working through the suggestions / changes here.

This is a big PR, but it enables @babel/parser to parse the Smart Pipeline proposal's topic reference. As discussed in babel/proposals#29, this is the first step towards enabling the pipeline plugin to parse and emit all three proposals.

Next, I'll be implementing F#-style parsing, then we'll work on the pipeline plugin itself. I want to get them in piece by piece to avoid merge conflicts. This work had drifted quit a bit, and took me a few days to refresh @js-choi's work with the latest from master, so I'd like to land each piece one by one.

This is a pretty major chunk of work, so definitely interested in getting as much feedback as possible.

🎉One step closer to the landing the pipeline proposals! 🎉

cc @littledan

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 8, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9434/

@nicolo-ribaudo
Copy link
Member

Major: Breaking Change? Maybe? (pipelineOperator in @babel/parser now requires proposal option)

Could you move it to a separate PR? Just in case this one doesn't get merged before the Babel 7 RC because it is a huge PR.

@mAAdhaTTah
Copy link
Contributor Author

@nicolo-ribaudo Done! #8291

@mAAdhaTTah mAAdhaTTah force-pushed the implement-smart-pipeline-in-parser branch from 12da3b9 to fbf62b4 Compare July 10, 2018 02:44
@mAAdhaTTah
Copy link
Contributor Author

Refreshed from #8291, so this is ready to review again.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser Spec: Pipeline Operator labels Jul 10, 2018
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

(I haven’t fully reviewed the PR yet, I didn't mean to approve 😅)

packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
packages/babel-parser/src/tokenizer/index.js Outdated Show resolved Hide resolved
packages/babel-parser/src/tokenizer/index.js Outdated Show resolved Hide resolved
packages/babel-parser/src/tokenizer/types.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
packages/babel-parser/src/tokenizer/state.js Outdated Show resolved Hide resolved
packages/babel-parser/src/types.js Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Member

Question: are x |> await foo and x |> (await foo) the same thing?

@mAAdhaTTah
Copy link
Contributor Author

Thanks for the review! As for the question

Question: are x |> await foo and x |> (await foo) the same thing?

No; x |> await foo is banned in every proposal, as its semantics are prone to confusion by users. x |> (await foo) desugars to:

const _temp = await foo;
_temp(x);

in every proposal. Smart pipelines enables x |> await foo(#), whereas F# enables x |> foo |> await. Both desugar to await foo(x).

@mAAdhaTTah
Copy link
Contributor Author

Just pushed fixes or responses for everything except fixing the Pipeline types, which I have to dig into.

Set whether we're in a pipeline in order to determine how to parse the
hash. The error message changes as a result, since the `hash` never
enters the block.
This ensures we clean up always if the callback throws.
This doesn't work because the `type` values are not compatible.
@mAAdhaTTah mAAdhaTTah force-pushed the implement-smart-pipeline-in-parser branch from baa971f to 39e7ee6 Compare July 22, 2018 22:59
@mAAdhaTTah
Copy link
Contributor Author

I addressed the comments so far, so this is ready for review again.

@mAAdhaTTah
Copy link
Contributor Author

@xtuc Updated as per comments, responded to some of your comments inline. Made a note to update the spec.md.

@mAAdhaTTah mAAdhaTTah force-pushed the implement-smart-pipeline-in-parser branch from e6465ef to 70318c9 Compare November 21, 2018 21:02
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Apart from some minor comments, this PR looks good to me!

packages/babel-parser/src/tokenizer/index.js Outdated Show resolved Hide resolved
};

export type PipelineStyle =
| "PipelineBareFunction"
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only non-node export of this file? I'm not sure that this is the right place where it should be put, but also I can't think of a better place 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, the file is called types.js so maybe it's ok?

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

👍 awesome work!

@thiagoarrais
Copy link
Contributor

@mAAdhaTTah: 🍾🍾🍾

@mAAdhaTTah
Copy link
Contributor Author

Thanks so much! I think @xtuc is the last holdout. Any remaining changes you need?

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

LET'S MERGE THIS 🎉 💃

@xtuc
Copy link
Member

xtuc commented Dec 3, 2018

Also thanks for your contribution and sorry if that took so long on our side (or at least mine)!

@xtuc xtuc merged commit fdc869c into babel:master Dec 3, 2018
@thiagoarrais
Copy link
Contributor

That's awesome, y'all! I can now get the PR for the transform going.

@js-choi
Copy link
Contributor

js-choi commented Dec 4, 2018

Thanks everyone for the hard work with the update and review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Pipeline Operator
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants