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

JavaScript: Support Smart and F#-style Pipeline #6319

Merged
merged 10 commits into from Apr 7, 2020

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Jul 20, 2019

Fix #6162

Support Smart Pipeline and F#-style Pipeline.

More about those proposals ("What's Happening With the Pipeline (|>) Proposal?").

Smart Pipeline:

// Input
5 |> # * 2

// Output (Prettier Stable)
SyntaxError: Unexpected character '#' (1:6)
> 1 | 5 |> # * 2
    |      ^

// Output (Prettier master)
5 |> # * 2

F#-style Pipeline:

// Input
promises |> await;

// Output
SyntaxError: Unexpected token (1:18)
> 1 | promises |> await
    |                  ^

// Output (Prettier master)
promises |> await;

ref: #7953

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

[
"decorators-legacy",
["pipelineOperator", { proposal: "minimal" }]
].concat(extraPlugins)
Copy link
Member

@duailibe duailibe Jul 22, 2019

Choose a reason for hiding this comment

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

This is bad. I added that as an escape-hatch until I could figure out a decent way to detect which decorator plugin to apply.

This will likely cause a noticeable performance degradation, specially when there's a simple syntax error.

Copy link
Member

Choose a reason for hiding this comment

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

@sosukesuzuki Why not always use proposal: 'smart'?

Copy link
Member Author

Choose a reason for hiding this comment

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

The smart is compatible with minimal, but it is coincidental. For example, the same problem will occur again when we try to support fsarp.

So, I considered generally solution...

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, we'll need to check only smart and fsharp then. Why check minimal?

Choose a reason for hiding this comment

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

smart is not compatible w/ minimal, so that's not a safe assumption. x |> foo() is invalid in smart but valid in minimal.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now. That's really unlucky. Hopefully Babel's error recovery might help here.

If it doesn't help, I'm going to make Prettier try using each proposal until the input parses. To avoid overhead, this logic will be used only if the input contains the substring |>.

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
@alexander-akait
Copy link
Member

In theory we can use babel api to detect plugin, if no plugins found use minimal

@thorn0
Copy link
Member

thorn0 commented Oct 1, 2019

Or we can suppress reparsing attempts if the file doesn't contain the substring |>.

@duailibe
Copy link
Member

duailibe commented Oct 3, 2019

@thorn0 That would be ideal

@mAAdhaTTah
Copy link

I helped implement the Smart & F# proposals in Babel. Is there anything I can do to help this land?

@thorn0
Copy link
Member

thorn0 commented Feb 24, 2020

@mAAdhaTTah Is there a quick and dirty way (like a regex) to detect which proposal was used in given code without full parsing?

@mAAdhaTTah
Copy link

@thorn0 Not that I can think of. Does prettier read the Babel config? That's probably the most reliable source of which pipeline operator is in use.

@thorn0
Copy link
Member

thorn0 commented Feb 24, 2020

That's the problem, it doesn't read it. And we'd like to avoid that as Prettier's configuration story is already complicated enough.

@thorn0 thorn0 changed the base branch from master to next February 24, 2020 21:52
@mAAdhaTTah
Copy link

@thorn0 How was this resolved for decorators? Similar approach, or was there something about the various decorator proposals that make this not an issue?

@thorn0
Copy link
Member

thorn0 commented Feb 24, 2020

@mAAdhaTTah The error recovery works really beautifully for decorators. Previously, we had to iterate through different option combinations to find the working one, but now (since v1.19) Prettier even supports different decorator variants within one file.

@mAAdhaTTah
Copy link

@thorn0 Cool alright, if there's anything I can do to adjust the error handling in Babel, let me know, but it might works as-is for this.

@thorn0
Copy link
Member

thorn0 commented Feb 24, 2020

foo |> bar() works (playground), even though proposal is set to smart.

But it doesn't recover from f# proposal syntax:

  • foo |> await |> bar (playground, await could be parsed as identifier in the error recovery mode)
  • foo |> bar => bar (playground)
  • something else?

@thorn0
Copy link
Member

thorn0 commented Feb 28, 2020

@mAAdhaTTah Could you make those errors recoverable, please?

@mAAdhaTTah
Copy link

@thorn0 I will look into that!

@sosukesuzuki
Copy link
Member Author

CI is failing due to memory problem, I may need to increase max-old-space-size or reduce maxWorker of Jest.
Please let me know if there is a better solution.

@thorn0 thorn0 added this to the 2.1 milestone Apr 5, 2020
@sosukesuzuki sosukesuzuki changed the title JavaScript: Support Smart Pipeline JavaScript: Support Smart and F#-style Pipeline Apr 5, 2020
@thorn0
Copy link
Member

thorn0 commented Apr 5, 2020

The memory problem happens because resolvePluginsConflict repeatedly modifies the same array, so it becomes huge.

Copy link
Member

@thorn0 thorn0 left a comment

Choose a reason for hiding this comment

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

Looks good. Could you add more code to the tests? E.g. from articles like this one.

@sosukesuzuki sosukesuzuki merged commit ccaaad0 into prettier:master Apr 7, 2020
@sosukesuzuki sosukesuzuki deleted the smart-pipeline branch April 7, 2020 10:42
node.body.operator === "|>")
);
}
return false;
Copy link
Member

Choose a reason for hiding this comment

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

This leads to a bug:

Prettier pr-6319
Playground link

--parser typescript

Input:

((x) => x) + '';

Output:

(x) => x + "";

}
}
}
return combinations;
Copy link
Member

Choose a reason for hiding this comment

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

Let's say the input combinations are [["jsx", "typescript"], ["typescript"]], in that case the output will be:

[
  ["jsx", "typescript"],
  ["typescript"],
  ["jsx", "typescript", ["pipelineOperator", { proposal: "smart" }]],
  ...
]

Why do we need the first two elements in this list?

@sosukesuzuki sosukesuzuki mentioned this pull request Apr 7, 2020
3 tasks
@sosukesuzuki

This comment has been minimized.

@kachkaev
Copy link
Member

kachkaev commented Aug 24, 2020

Hey folks sorry for a late comment here. Just a question on:

- [Link to the 'smart' pipeline proposal](https://github.com/js-choi/proposal-smart-pipelines)

Any reason to link to the fork (67 stars) instead of https://github.com/tc39/proposal-pipeline-operator (4.8K stars)? If no specific reasons, WDYT of changing the link in the 2.1 blog post while it's fresh?

@thorn0
Copy link
Member

thorn0 commented Aug 24, 2020

@kachkaev There are three different pipeline operator proposals: "minimal" (the one you linked to), "f#", and "smart".

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipeline operator: support Smart & F# proposals
7 participants