RxJS pipe chaining get formatted on a single line

[yannickglt]

Prettier 1.11.1
Playground link

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));

Expected behavior:
As in the official RxJS documentation, the given input shouldn't be modified as it is considered as methods chaining. The piped functions filter, map and scan should remain on a separate line.
I don't know how it can be handled by prettier without too much impacts, but maybe considering that a function call that includes at least 2 function calls as parameters (eg: fn1(fn2(), fn3())) could be printed as a chain as below:

fn1(
  fn2(),
  fn3()
)

It would also help when using Lo-Dash FP chaining which encounters the same problem.

[suchipi]

Prettier is not distinguishing this from a normal function call with three arguments, which is why this is formatted as you see. We could pursue special formatting for RxJS as a new feature if there is demand for it but I haven't seen much yet.

[yannickglt]

Thanks for your answer. Do you want me to close the issue or you prefer let it open in order to collect votes?

[SamVerschueren]

I agree with @yannickglt. As a developer it's much easier to reason about an RxJS pipeline if every step is on a separate line.

maybe considering that a function call that includes at least 2 function calls as parameters (eg: fn1(fn2(), fn3())) could be printed as a chain as below:

This is also what popped in my mind as well. If I only have one step in my pipeline, for instance a map operator, I write it as pipe(map(() => 'foo')). But whenever I have more operators, I spread them out over multiple lines. So I'm not sure if this is something RxJS specific or should be implemented that way.

[suchipi]

@yannickglt let's leave it open to collect votes.

[kylecordes]

This is one of a set of similar cases where it is helpful to understand the semantics of the code a bit when formatting it. Because the formatting then makes them easier for the next reader to understand. I don't know if there are any features that currently do this in prettier, or if maybe this might be the kind of problem for a future generation formatting tool to solve. Such a thing would not look only at the current file, but would look at the types of the data, and could be configured (or, in the box) with some rules that modify the formatting according to the types.

[appsforartists]

clang-format uses trailing commas as a hint that you want each argument on its own line. Works for array literals, object literals, function signatures, and function calls. Would be nice if prettier did the same.

[pringshia]

Rather than a RxJS-specific solution, what about providing a hint to prettier for such use-cases? Maybe the ability to specify in the configuration file (i.e. .prettierrc or prettier.config.js) which function calls should always list their arguments on a newline. The functions can be identified by name "pipe". This isn't the strongest link and may cause naming conflicts. Not sure if there is a better way to ID.

Another alternative can be to allow for eslint-type comment hints (e.g. // eslint-disable-line no-use-before-define) however, I'm not a huge fan of artifacts of the development process being commingled with source code and this would have to be done every function invocation.

[aboyton]

I completely agree that putting these on one lint is worse, but this problem will go away with https://github.com/tc39/proposal-pipeline-operator

There's still some discussion about the precedence of the operators (tc39/proposal-pipeline-operator#23 (comment)) but I believe this code will look something like this.

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

const source$ = range(0, 10);

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

Given that this proposal is in the works is it worth hardcoding manual pipes while we wait?

[jayphelps]

@aboyton Definitely! But there's a fair chance pipeline will never get added and if it does it will be quite a while before it's stage 3 (enough for most people being OK to use it in babel, though some will still wait for stage 4).

IMO we ultimately should continue as if pipeline won't ever be standardized.

[appsforartists]

Making this specifically about RxJS feels like a red herring. The bigger question is "should there be a hint that comma-delimited values each get their own line?" My vote is yes, and that the hint is a trailing comma. It's simple, consistent with other tools, and completely valid JS (even in the absence of a tool like Prettier).

[kylecordes]

It seems to me that such hinting is desirable. I don't think the upcoming pipeline operator takes care of it - the pipeline operator may not (in fact probably should not) imply vertical alignment. Fundamentally the pipeline is just a different way of chaining operations together; plenty of times we chain operations now using ".", and there certainly isn't a rule that "." implies vertical alignment.

Ideally hinting could be based on the type system, as I hinted at an earlier comment. Thus rules like "when a function or method call accepts a spread ("...") of functions, align them vertically always, never on the same line". However, I don't know the internals of prettier, if reaching the type system is too far out of its grasp then perhaps a more expedient solution is warranted.

[SamVerschueren]

Hinting newlines with a trailing comma could conflict with the trailing-comma option. If this is set to none, it will remove the trailing comma from the list and the pipe arguments will again be spread out over one line.

[suchipi]

We don't want to rely on "hinting" at all if we can avoid it. The fact that you can hint whether objects should be single-line or multi-line is considered a bit of a wart by the maintainers, but we haven't found a better solution since objects are used in a variety of ways.

As such, hinting from commas for multiline calls will not be pursued. Let's keep this discussion around RxJS specifically- supporting popular libraries is not unheard of in prettier (we already support styled-components, for example).

If we add special-casing for RxJS, given how common its function names are, I think we will need a more rigorous solution than "identifier equals pipe"- maybe we could rely on observable values ending with a dollar sign, or detect an RxJS import/require somewhere in the file. I'm not using RxJS a ton so I'm not familiar with the common conventions, so I'd appreciate input from the community here.

[SamVerschueren]

As I said before, is this really something that is RxJS specific? Or should be implemented that way? Couldn't it just be an option which set to true will spread function calls inside another function call on multiple lines?

For example

fn(foo(), bar(), unicorn());

/// will result in

fn(
  foo(),
  bar(),
  unicorn()
);

I believe that if you want spread them out over multiple lines in RxJS, you are also in favour of spreading the previous example out over multiple lines.

Might be missing something here though, just my two cents.

[duailibe]

@SamVerschueren This is a great suggestion. We need to run that in some codebases to see how it plays out.

This won't fix the "Lo-Dash FP chaining" case mentioned in OP though, since it seems to accept identifiers as well:

compose(
  sortBy(x => x), 
  flatten, 
  map(x => [x, x*2])
);

Are there any RxJS operators that don't follow this () => Observable => Observable pattern ?

[Phoenixmatrix]

IMO this is a bigger issue with long line lengths (if you follow Prettier's heavily recommended 80 characters, you can't go very far before Prettier starts putting 1 function by line, and if you only have 2 operators in the pipe, it doesn't read too bad. Not optimal, but not bad).

Otherwise, this would have to go by heretics. Something like "if all arguments to a function are themselves function calls, where at least 1 contains a callback, make them one by line" (this is a naive example, so take it with a grain of salt).

That way foo(bar(), baz()) stays the way it is (as it, IMO, should), but foo(bar(x => x * 2), baz()) becomes

foo(
 bar(x => x * 2),
 baz()
);

[SamVerschueren]

This won't fix the "Lo-Dash FP chaining" case mentioned in OP though, since it seems to accept identifiers as well:

That's also possible in RxJS. Actually, that's just always possible.

const fooSomething = foo();

fn(
  fooSomething,
  bar(),
  unicorn()
);

My suggestion would be to always spread on multiple lines if both of the following statements are true:

1. We have more then 1 argument
2. One of the arguments is a function

Examples

// These won't spread on multiple lines
fn(foo());

fn(fooSomething);

fn(fooSomething, barSomething);

// These will be spread on multiple lines
fn(
  foo(),
  bar()
);

fn(
  foo(),
  barSomething
);

fn(
  fooSomething,
  barSomething,
  unicorn()
);

// Special case?
fn(
  foo(
    unicorn(),
    rainbow()
  )
)

[benlesh]

FWIW: Google uses clang format internally, and it uses a trailing comma to hint whether to make everything one line or put each item on a new line. It works fantastically and it's intuitive. This is something that is company-wide at Google, so thousands of developers know how it works and I've never once heard a complaint about that specific behavior, if anything it's appreciated because it gives the developer control over this aspect of readability.

I wouldn't want there to be a special rule for RxJS pipe, because in some cases you still want everything to be one line:

// this is so short, I don't think it hurts readability to have it in one line,
// nor do I think having multiple lines would help readability.
const x = foo.pipe(map(fn1), filter(fn2));

But in the majority of cases, RxJS users will want their operators each on a different line.

[SamVerschueren]

@benlesh the trailing comma hint was suggested above as well, I answered why I believe it's not a good idea as it collisions with the trailing-comma option.

[appsforartists]

There could always be another option, trailing-comma: manual.

Bikeshedding the name is a separate conversation. It's good to know about that option, but it doesn't prevent using trailing commas to denote newlines either.

[suchipi]

One of the project goals of prettier is to remove decisions around formatting from the user, so that they (and their team) can focus on the code. Instead of worrying about formatting whitespace while coding, you just write. Instead of arguing about formatting in pull requests, you just look at the code. There is a lot of benefit to removing the cognitive question of styling.

Hinting seems like a step in the wrong direction. We currently accept hints for object literals (object literals containing newlines get formatted as multiline even if they would fit on one line), and it mostly works, but it is annoying that it can cause prettier to format the same code differently.

When prettier doesn't do what you'd do and you waste time adjusting the object literal so that prettier will format it differently, I consider that a failure to meet the goal of eliminating the decision-making process around formatting. On the other hand, if prettier formats something differently than I would have, but I can't influence that formatting, I will accept it and move on. I prefer that to the sort of wishy-washy behavior that hinting enables, which in my opinion goes against one of the strongest draws of the tool.

I would prefer to focus on adding specific, scoped support for RxJS that provides a good experience, rather than trying to implement broad-stroke changes that compromise the goals of the project.

[kylecordes]

@suchipi That seems like a very wise strategy. However I really (really) hope that if this works out, it's not hard coded specifically to RxJS but rather either detects the pattern or can be configured.

RxJS is an obvious example almost everyone has heard of, but there are various other libraries that follow that same pattern, it would be a real bummer to have code using those libraries get uglier while code using RxJS gets Prettier.

[suchipi]

If there is a non-hinting-based heuristic that can help other libraries (like lodash, redux, etc), then I think it is worth implementing. However, if there are situations which are too ambiguous to determine via heuristics, and those situations are less ambiguous if we are aware that the user is using RxJS, then I think detecting RxJS in that situation is worth considering.

[kylecordes]

@suchipi I suggested something earlier, but it depends on understanding the types... And I don't know whether Prettier actually understands types or not. Here was the suggestion, cleaned up a little bit:

Whenever there is a method call which accepts two or more function arguments (or a ...spread of function arguments), regardless of how many are actually being passed, always stack them up vertically.

This would catch the RxJS Pipe, a similar construct in Lodash, and various other libraries - without knowing anything about those libraries, not even their names.

---

There is a harder case, not as amenable to a generic solution, that I brought up elsewhere. I don't have any kind of type based rule for this one. In Angular, when defining a NgModule, wise developers (wisdom earned by merge conflict tedium) tend to always stack up their arrays of declarations, imports, exports, etc. vertically in a manner that minimizes merge conflicts. It would be most excellent if Prettier would respect this convention also - but the only way I can think of for that is to specifically recognize a Angular NgModule :-(

(Fortunately I think this harder case will eventually go away when we all commonly use language aware merge algorithms, which can merge concurrent edits to an array without regard to how the array was formatted in the source code.)

[suchipi]

Prettier doesn't know types, only syntax. So it can tell bla and bla() apart, but otherwise doesn't know if a value is a function.

[vjeux]

It seems like pipe(a, b, c) and compose(a, b, c) are the two places where you always want to break arguments into new lines. Is that correct?

If that’s true, it’s not unreasonable to hardcore those two names inside of prettier. We already do for unit testing and a few places.

Would that solve the problems that people in this thread are facing?

[vjeux]

Also, this thread is very light on real snippets of code, it would be nice if everyone that is affected could provide real before/after examples. We have found that it’s very hard to come up with good rules with pseudo-code.

[suchipi]

@vjeux lodash also has "flow" and "flowRight". I dunno if we want to hardcode "flow" since it's a common word, but maybe "flowRight".

[joshburgess]

@vjeux Yes, I think hardcoding the proper behavior is perfectly fine if you all are willing to do it.

However, I think adding a few aliases would be required to cover all common names/libraries.

For me, pipe (forwards composition) and compose (right-to-left composition) are the two most common names.

However, it should be noted that lodash uses the terms flow and flowRight also.

Other than that, the only other term I think you'd see, (and it's less common, but still), would be composeFlipped, as that's what you'd see it called in Haskell, PureScript, etc...... it's synonymous with pipe or flow.

I think these few aliases would cover the vast majority of cases.

Another one to consider that behaves similarly is partial, which is found in ramda & is sort of a one-time use way to do partial application that isn't auto-currying. Something like the below:

const partial = f => (...args) => (...rest) => f(...args, ...rest)

Come to think of it, it's probably any common variadic utility function that takes any number of arguments.... not sure we could cover all cases without a rule like using trailing commas as a signal, but hardcoding the most common cases would still be useful and appreciated.

[joshburgess]

Haha @suchipi beat me to it with the aliases.

[vjeux]

@suchipi are you interested in implementing this heuristic with all those names? It sounds unlikely that it's going to affect anyone else (including for flow) and given that this issue has 156 likes it would help a bunch of people.

Thanks @joshburgess for all the context! Do you happen to have examples of calls of partial that we can figure out better heuristics for it.

[joshburgess]

@vjeux I don't have an example off the top of my head, as I don't really use it much... but it's basically the same issue as with compose/pipe. They are variadic (they take a variable number of arguments). So, when passing them more than 2 or 3 args, they may extend to multiple lines... this can cause prettier formatting that doesn't look so great, particularly when it puts some of the arguments on one line and more on the next.

This mostly likely is the same with other variadic functions out there, like redux-observable's combineEpics and redux's combineReducers, but I'd have to check to be sure.

Maybe, Ben Lesh's idea about using trailing commas as a signal to always use new lines for each arg is a more all-encompassing solution?

The only issue there is that you could only rely on it for ES6... and it's not great to exclude the feature from ES5 users, but I can't think of a solution to that problem at the moment.

[vjeux]

The way clang-format handles this problem is interesting but I believe that it would work against prettier's value proposition: to reduce the time people spend formatting their code. If you give people control over function arguments being inline or multiline, this means that now people have to explicitly think about formatting when they code. Also, now you have two different ways to write the same piece of code so you're going bring back all the discussions around style, which we tried to eliminate in the first place...

The good news is that the problem is very localized: this appears to mostly be an issue for calls where functions are common functional programming combinators. Whitelisting them makes a lot of sense, this will properly solve the problem, it is very unlikely to have unintended side effects and the implementation is trivial. I believe that this is the best course of action, instead of trying to come up with a general way to format functions that will have a lot more consequences to prettier.

[suchipi]

@suchipi are you interested in implementing this heuristic with all those names? It sounds unlikely that it's going to affect anyone else (including for flow) and given that this issue has 156 likes it would help a bunch of people.

@vjeux can do

[suchipi]

Opened a PR: #4431

It supports:

• pipe
• pipeP
• pipeK
• compose
• composeFlipped
• composeP
• composeK
• flow
• flowRight
• connect

I added in the P/K variants for Ramda.

[suchipi]

@benlesh are there any other RxJS operators we should include support for?

[tusharmath]

@suchipi @vjeux If the way forward is to understand function names and then format, then we should probably reopen the hyperscript formatting issue here — #1949.

[suchipi]

@tusharmath in this case, the function names are very specific, so there is a low chance of overlap with people using these function names in places where this formatting does not make sense. But I'm not 100% sure that the same safety is present with hyperscript (though I could be persuaded otherwise). #4424 might be a better approach for hyperscript, because then the functionality will be opt-in, which lowers the chance of adverse effects.

[joshburgess]

Good call, @suchipi . There are a few other libraries which have things like composeP, etc. also. 👍

[j-f1]

How about matching compose[A-Z]?

[suchipi]

The letters aren't arbitrary; the P stands for Promise and the K stands for "Kleisli composition". I don't think it would hurt anything to match A-Z but I don't think anyone would benefit from it either.

[rockymadden]

@suchipi As mentioned on twitter, there are also other compose-centric functions like Ramda's o function which a stricter version of compose. However, after looking over what's being done I am not sure if it would warrant inclusion. I was also looking through Sanctuary (https://github.com/sanctuary-js/sanctuary), partial.lenses (https://github.com/calmm-js/partial.lenses), Crocks (https://github.com/evilsoft/crocks), and others -- it might get too complex fairly quickly if also wanting to prevent false positives.

[suchipi]

@rockymadden thanks for the help! Since R.o only takes two parameters, I'm going to leave it out for now. As for the others, I'm going to leave them out of #4431, too, since the libraries are less popular and I'm not sure this heuristic will apply to them broadly. However, if community demand arises for formatting changes in those or other functional libraries, I'd be open to extending this heuristic or pursuing a different one at a future time- just open an issue

[duailibe]

I'm sorry for pinging everyone in this thread but here's where most people concerned about this formatting were involved. I just want to give a heads up that we are changing this, trying to improve things for you and other users.

After this change, we've received lots of issues commenting simple code samples were being formatted strangely:

// the single line version is preferred here
connect(
  foo,
  bar
);

This showed that the solution we adopted was not ideal... while we were fixing this the issue in this thread, we were potentially causing churn to lots of other users.

For that @brainkim just finished a PR that'll remove the hard coded functions that are forced multiline, and instead, we'll use a heuristic that will consider the number of functions passed to the arguments. You can find more details in #6033.

I'm keeping this issue locked and anyone can voice their opinions on the PR. Thanks for the patience!