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

Add hack style pipeline proposal #11600

Closed
wants to merge 10 commits into from
Closed

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented May 23, 2020

Q                       A
Fixed Issues? Addresses tc39/proposal-pipeline-operator#84
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link (I'll add this shortly)
Any Dependency Changes?
License MIT

This addresses the discussion starting at tc39/proposal-pipeline-operator#167 (comment).

It adds the Hack-style only pipeline proposal. It's a strict version of the smart proposal, it's simpler, and I hope it has a better chance to go through the proposal process.

Even if this isn't your favorite version, or you don't believe this is the right path forward, surely we can agree it would be good to level the playing field to have this proposal implemented in Babel.

Implementation details

I dropped the indirect eval. I don't see a reason to special eval for pipelines, the point of this proposal is that it is extremely simple and interacts well with existing code. Therefore this code:

var x = 5;
return "console.log(x)" |> eval;

should work just as well as

var x = 5;
return eval("console.log(x)");

Tests

I converted the "bare" test to an error test (the other proposals are unfortunately missing tests for errors, so the coverage there isn't great).
I changed the eval test to test the "direct" eval.

@aadamsx
Copy link

aadamsx commented May 23, 2020

More examples please.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 23, 2020

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 da02613:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented May 23, 2020

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

@xixixao
Copy link
Contributor Author

xixixao commented May 23, 2020

@aadamsx sorry, what do you mean by more examples? This PR includes a full test suite. What other kind of example would you like to see?

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Pipeline Operator labels May 23, 2020
@mAAdhaTTah
Copy link
Contributor

To clarify: Hack is Smart without "bare style"?

@nicolo-ribaudo
Copy link
Member

According to the linked issue, it seems that hack doesn't use # but binds the piped value to the $ binding 🤔

@nicolo-ribaudo
Copy link
Member

@tabatkins - RE: tc39/proposal-pipeline-operator#167 (comment)

Do you plan to talk about this during the June meeting? I tried looking for slides or examples in the agenda but I couldn't find them.

@xixixao
Copy link
Contributor Author

xixixao commented May 26, 2020

@mAAdhaTTah correct, this is like the "smart" proposal but without supporting "bare" calls.

@nicolo-ribaudo $$ as topic reference works for Hack, but clearly doesn't for JS. # seems to be the most likely candidate, and it's already used for the smart proposal.

@tabatkins
Copy link

Talking with the other pipeline champions, we'll be targetting the July meeting for pipeline discussions.

@js-choi
Copy link
Contributor

js-choi commented Apr 5, 2021

There’s now a formal Hack-pipes proposal, which is replacing the now-withdrawn smart-mix pipes proposal. The next steps are:

  1. First we will need to resolve this fork’s conflicts with Babel latest.
  2. Then we will need to adjust its code to match the new formal specification, which has a bunch of grammatical simplifications.
  3. We also will need to remove smart-mix functionality, since smart mix’s proposal has been withdrawn.
  4. After this pull request gets merged into Babel latest, we should try to add support for different placeholder tokens like % and @, in another pull request.

I’ve been talking with @xixixao, @mAAdhaTTah, and @tabatkins about this. We’ll work on this when we can.

@nicolo-ribaudo
Copy link
Member

Thanks!

Unwrap and remove the PipelineBody,
PipelineBareFunctionBody,
PipelineBareConstructorBody,
PipelineBareAwaitedFunctionBody,
PipelineTopicBody,
and PipelineStyle types.
The “primary” part was a vestige of the old smart-mix
pipeline proposal.
@xixixao
Copy link
Contributor Author

xixixao commented Apr 9, 2021

Updated to @js-choi 's new version of this PR.

@js-choi
Copy link
Contributor

js-choi commented Apr 9, 2021

Thanks, @xixixao.

The current code now should have no conflicts with Babel upstream. In addition, the obsolete smart-mix plugin mode has been removed. ASTs also have been simplified; they just use BinaryExpression without any extra node types.

Before this gets merged, I think it would be a good idea to change the Hack plugin mode to require that a topic token be specified in its configuration, with no default token. (The default topic token is currently #, which had not been well received by TC39 in the past.)

To that end, I’m working on adding support for specifying the topic token right now, after which I’ll work on supporting @, %, and ?.

I’m working on these changes in my own fork at https://github.com/js-choi/babel/tree/proposal-hack-pipes.

@nicolo-ribaudo
Copy link
Member

Can we keep adding the hack proposal and removing the smart one in separate PRs? We cannot remove the smart proposal until the next major, but the hack proposal can be implemented in a minor.

@js-choi
Copy link
Contributor

js-choi commented Apr 9, 2021

Can we keep adding the hack proposal and removing the smart one in separate PRs? We cannot remove the smart proposal until the next major, but the hack proposal can be implemented in a minor.

Alright, noted; I’ll add smart-mix mode back in a future commit.

js-choi added a commit to js-choi/babel that referenced this pull request Apr 9, 2021
js-choi added a commit to js-choi/babel that referenced this pull request Apr 9, 2021
js-choi added a commit to js-choi/babel that referenced this pull request Apr 9, 2021
js-choi added a commit to js-choi/babel that referenced this pull request Apr 9, 2021
js-choi added a commit to js-choi/babel that referenced this pull request Apr 14, 2021
js-choi added a commit to js-choi/babel that referenced this pull request Apr 14, 2021
@xixixao
Copy link
Contributor Author

xixixao commented Apr 24, 2021

Superceded by #13191

@xixixao xixixao closed this Apr 24, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2021
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 PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Pipeline Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants