Do not react when MemberExpression object type is ThisExpression or Super
Filters out cases like

👍

Do not react when all arguments are Identifiers and parent node is not CallExpression with callee equal current node.

I don't think we should do this, like @duailibe pointed out, it's a valid pattern

Hi @duailibe @suchipi, thanks for being so responsive here!

1.

I agree that

const someFn = pipe(
  firstStep,
  secondStep,
  thirdStep
);

is a valid pattern in a functional context, but it arguably doesn't improve readability much over

const someFn = pipe(firstStep, secondStep, thirdStep);

I think leaving is as-is, provided it can fit on one line, is actually more in-line with the Prettier ethos and people's mental model of how Prettier works. Splitting it into multiple lines in the above case is surprising.

This was also mentioned in #4172. My aim here is to help improve the 'is this functional' heuristic by looking at the shape of the code.

2.

You might want to call the result of the composition several times so it's a valid reason for it not to be the callee of a call expression.

Correct, but that condition is used to prefer

const ArtistInput = connect(
  mapStateToProps,
  mapDispatchToProps,
  mergeProps
)(Component);

over

const ArtistInput = connect(mapStateToProps, mapDispatchToProps, mergeProps)(
  Component
);

In the case of an intermediate variable, it would be just

const connectComponent = connect(mapStateToProps, mapDispatchToProps, mergeProps);
const ArtistInput = connectComponent(Component);

Which leads us to the previous case.

I'm happy to add more specific cases like Redux's connect if that would be useful?

3.

I've also just added literals to that condition, so things like:

compose(1, 2, 3);

won't become

compose(
  1,
  2,
  3
);

Please take a look and let me know what you think :)

@princed despite keeping it inline being more consistent with prettier's philosophy, users of RxJS in particular voiced that they want pipe to keep everything on multiple lines, so that's behavior I want to keep for identifiers. However, I'm fine with changing it for literals (not object literals though).

Maybe we should not do the line breaks with identifiers for connect, but do it for pipe and etc?

users of RxJS in particular voiced that they want pipe to keep everything on multiple lines

Couldn't those composition function names be configuration based, so we aren't making any assumptions about which method names a particular codebase considers functional?

I know prettier is meant to be light on configuration, but catering the formatting to users of a particular library seems a bit strange to me as well.

+1 for this being surprising behaviour — it doesn’t seem consistent with how Prettier has worked until this point: which is to keep things on one line if it can fit into the print width.

Maybe we should not do the line breaks with identifiers for connect, but do it for pipe and etc?

I think that’s still open to false-positives. What if you happened to have a method called ‘pipe’ in one of your classes?

I’d love to see a compromise here which isn’t tied to specific method names, something like:

• the identifier behaviour you mentioned above

• just keeping it on one line if it fits

• a ‘break method names’ list option like @marcfallows describes above, so that people can opt-in to this behaviour and it’s not a hard-coded set of method names for a mix of libraries that not everyone uses.

This last one would also shift the burden of listing the names to users, and Prettier won’t need to be updated each time RxJs, Ramda, etc add or rename methods. You also wouldn’t end up with further PRs to add more names to this list for miscellaneous future libraries, and any false positives would also be something the user has chosen to live with.

Thanks!

I suppose I interpreted the 176 👍s and long discussion on #4172 as being less about a particular library such as RxJs or Ramda and more about the programming concept of functional composition, which very often is paired with function names of compose, pipe, etc.

Maybe it would be useful to bring some of the original commenters from #4172 in here for further input...

A lot of the 👍s on #4172 appeared when Ben Lesh tweeted something to the effect of ":+1: here to support better formatting for RxJS in prettier" and linked to the issue.

@suchipi I would love to know which part of my comment made you give the 👎, and why. Could you elaborate?

@marcfallows it wouldn't be appropriate to add a configuration option for something so small, especially when we've turned down configuration options for changes with much more impact. See also https://prettier.io/docs/en/option-philosophy.html

@duailibe @suchipi
this filtering is split out to #4836, PTAL there

I'm discovering this pr because upgrading prettier is modifying thousands of function statements in our app to be multiline when they are short, clear and well within the line length. We have it filled as a bug in our system, one that's blocking upgrade.

I hope prettier doesn't end up reading my function names, that could get wired quick and I'm happy to see this land.

@suchipi @duailibe for the other (non-this) cases, would it be possible to apply a strategy of:

• leave it on one line, if the statement fits in the print width
• if it doesn't, then apply any special functional heuristics

I think that would be less surprising and more in line with how prettier works for all other cases

It would also avoid most of the false-positives seen by us and by @reconbot

@alextreppass in the first post of the original PR (#4172 (comment)) @yannickglt made the point that the print width is not enough for functional composition. Consider the following:

Input

import { range } from 'rxjs/observable/range';
import { map, filter, scan } from 'rxjs/operators';

const source$ = range(0, 10);

source$.pipe(
  filter(x => x % 2 === 0),
  map(x => x + x),
  scan((acc, x) => acc + x, 0)
)
.subscribe(x => console.log(x))

Output

import { range } from "rxjs/observable/range";
import { map, filter, scan } from "rxjs/operators";

const source$ = range(0, 10);

source$
  .pipe(filter(x => x % 2 === 0), map(x => x + x), scan((acc, x) => acc + x, 0))
  .subscribe(x => console.log(x));

The output line with .pipe here is only 80 characters, but difficult to visually parse to figure out what is going on.

Some other points were made in that issue too, probably more eloquently and comprehensively than I can.

@karlhorky thanks for the context. I agree the pipe line in that example is noisy.

I'm just frustrated that something as terse as

foo.compose(d1, d2)

gets transformed to:

foo.compose(
  d1,
  d2,
);

I'm hoping that there's a way forward that keeps the functional composition people happy, while not ruining simple cases that are already fine on one line.

@suchipi @vjeux any chance you can re-consider the Identifiers approach that @princed came with above? Or suggest some other way we can make the one-line cases less awkward?

Maybe we should scope down eg foo.compose to only R.compose, etc from popular functional libraries, instead of any member expression. We would need to do some research to find common usage patterns in the wild among popular libraries.

I use bluestream and streaming-iterables, they're small packages with only a few hundred users but I'd much rather prettier only modify the call signature if it's too long.

import { pipe, map } from 'bluestream'
import { collect } from 'streaming-iterables'

const transform = map(async () => 'foo')

async function foo() {
  await pipe(ExtractStream, transform, LoadStream)
  await collect(asyncIterator())
}

to

import { pipe, map } from "bluestream";
import { pipe, map } from "bluestream";
import { collect } from "streaming-iterables";

const transform = map(async () => "foo");

async function foo() {
  await pipe(
    ExtractStream,
    transform,
    LoadStream
  );
  await collect(asyncIterator());
}

After 2nd thought I think we should merge this.. it's still an improvement for several cases asked in the original thread. It's a trade-off and I'm more comfortable not breaking for FP users than breaking for non-FP users.

@suchipi

@duailibe I don't want to introduce back-and-forth churn for the (silent) users that were excited for this change in 1.13. I think it might finally be time to start checking scope to see if stuff is imported from a named package... and we should probably find a way to make that work with some kind of plugin API.

(For what it's worth, I am also affected by these false positives at work, so I don't intend to ignore this forever- but I think we need to be smarter about how we do it.)

🤷‍♂️ I'm all for doing it right.

If you're a user who's using code that share method names with functional libraries you can lock your prettier to "1.10.2" to avoid getting all of these false positives.

As I commented here it might be simpler do something a little different.

@benlesh This approach of making commas significant to prettier's decision of multiline vs inline would add manual steps for achieving certain formatting during editing code, which feels like it's against prettier's philosophy.

I think @jlongster has argued for this well in #3503 (comment):

I'm certainly biased; it gives me great pleasure if I have a function whose args have been broken up:

function compile(
  parser,
  shouldExtendScopes,
  lazilyCompile,
  otherOption
) {
  // ...
}

And I remove otherOption and it collapses it back:

function compile(parser, shouldExtendScopes, lazilyCompile) {
  // ...
}

Removing that functionality would be a big loss IMHO.

Like others, I’m finding this PR after seeing simple things like

@connect(null, actions)

suddenly get broken into multiple lines. I really hope Prettier will revert back to the old behavior, and just let the fp crowd configure whichever methods they want aggressive breaking on. Guessing where to do this seems impossible, and an ideal use case for userland configuration.

@karlhorky I'm sorry, I don't think the argument is effective, because I think how people want these things broken up is too subjective for an algorithm to make good choices. A human decision is going to be better here. @arackaf's problem just above sort of proves my point. This is literally never going to make people happy in all cases until they have some form of control over the behavior on a case-by-case basis.

I'm closing this one since it's unlikely to be merged.
Happy to bring it back to life if the need arises.

Any hope of getting an option added so decorator users will stop having code mangled? Between this, and another recent change, decorators are becoming almost unusable with Prettier. Even something so crude as to turn all formatting off for decorators, except for basic alignment would be a godsend.

@arackaf See #4922 (comment)

@lydell - yes! That’s amazing. I was really really hoping some hero would come along and do that PR.

Any ETA on a release???

I think #5259 is the biggest thing that needs to be merged before we can release.

That’s unfortunate to hear. There’s no way to just get a patch release pushed with these decorator fixes, with Angular/Vue enhancements coming sometime later?

Right now the code for angular/etc is already in master, so if we released the branch as-is, users would use it before it's ready, which would cause issues. We would have to branch to make a release that disables HTML support, but for a release this big (it's been months), we usually want to write detailed release notes so people understand what changed, and the HTML stuff would probably be done before we finished the release notes. So, just a little bit of patience, please- we are eager to get these fixes out, too 🙏

Ah - understood. Thank you for taking the time to explain. Will be worth the wait - thanks a ton for all you guys do!