Prettier: Revisit function literal heuristic introduced in Prettier 1.19

[rassie]

Prettier 1.19.1
Playground link

--parser babel
--print-width 100
--single-quote

Input:

stream1$.pipe(
  withLatestFrom(stream2$),
  distinctUntilChanged(),
  debounceTime(50)
)

Output:

stream1$.pipe(withLatestFrom(stream2$), distinctUntilChanged(), debounceTime(50));

Expected behavior:
Function literal heuristic introduced in https://github.com/prettier/prettier/pull/6033 is insufficient for RxJS, since it ignores a lot of operators which at their core are functional compositions, but not in syntax. I don't have a solution proposal, but it's breaking existing (and expected) behavior right now.

[thorn0]

The output looks perfectly readable to me.

[rassie]

Yes, as a separate expression. However, a shorter expression with a function literal in it would be formatted in multiple lines, so we have a situation when sometimes a piped operator is in the same line and sometimes not. I'd argue it's more difficult to read this way, since it's inconsistent and it's not obvious for the reader whether line-scanning is needed or not. I can provide an example in about half an hour, maybe it'll become clearer what my problem is.

[rassie]

Here's an example, shorter expression with line breaks and longer in one line:

Input:

stream1$.pipe(
  map(x=> x*2),
  distinctUntilChanged(),
  debounceTime(50)
)

stream1$.pipe(
  withLatestFrom(stream2$),
  distinctUntilChanged(),
  debounceTime(50)
)

Output:

stream1$.pipe(
  map(x => x * 2),
  distinctUntilChanged(),
  debounceTime(50)
);

stream1$.pipe(withLatestFrom(stream2$), distinctUntilChanged(), debounceTime(50));

[brainkim]

@rassie So, perhaps your main issue isn’t that putting the pipe calls on a single line are unreadable, but that the current heuristic creates inconsistencies in formatting two call expressions of similar length. In other words, you either want both of the following snippets to be formatted on the same line or both to be split?

stream1$.pipe(map(x => x * 2), distinctUntilChanged(), debounceTime(50));

stream1$.pipe(withLatestFrom(stream2$), distinctUntilChanged(), debounceTime(50));

If you look at the single-line forms of the two pipe expressions above, I think there’s an argument to be made that the first call expression is slightly less readable because of the anonymous function. Maybe we can make it so call expressions can tolerate a single nested function literal without breaking onto multiple lines? I chose to break immediately when we found any rxjs-like operator calls, because I thought this would have made the change slightly less disruptive, but maybe we can increase the tolerance to >1 function literals consistently?

PS, this is unrelated but I think there’s a bug in your code where distinctUntilChanged requires a comparator, because withLatestFrom creates an observable of tuples, but distinctUntilChanged uses referential equality so every update is essentially marked as changed.

[rassie]

Yes indeed, it's about consistency, and I strongly prefer multiple lines over single-lines for operator pipelines (aka Prettier 1.18), because it's easier to reason about them, reading from top to bottom. A possibility to enforce that would be very nice :)

(yes, there is probably a bug in the code, however, that's just placeholder code and not something in my codebase. But thanks anyway!)

[brainkim]

I strongly prefer multiple lines over single-lines for operator pipelines (aka Prettier 1.18), because it's easier to reason about them, reading from top to bottom.

Ah so it’s not necessarily about consistency but preference for the old behavior. I think you’re in good company, and a lot of people feel the way you do, however the question I pose to you, is from an implementer’s perspective, how do you distinguish the call expression you posted above from any other call expression? The old behavior relied on reading the function name and hard-coding a set of names to split automatically, but this was surprising behavior (see any of the linked issues on the PR). I’m not sure there is a way to distinguish your call from any other call expression without actually reading the type information of the function or its arguments.

One alternative is to place a line comment somewhere in the call-expression to force a split:

stream1$.pipe(
  //
  withLatestFrom(stream2$),
  distinctUntilChanged(),
  debounceTime(50)
);

[jrnail23]

It was clear from the release notes that the intent here was to consistently split functional composition across multiple lines, as that's simply how people write functional code.

Upgrading to prettier v1.19 threw me off for similar reasons, as my composed functions might not be made up of function literals.

[larskinn]

I find this new behaviour very surprising. Here's an example which in which formatting a nested RxJS pipe is inconsistent with the outer pipe:

Prettier 1.16.4

const searchEpic = action$ =>
    action$.pipe(
        filter(isActionOf(Actions.search.request)),
        debounceTime(500),
        switchMap(action =>
            from(
                doSearch(
                    action.payload.firstName,
                    action.payload.middleName,
                    action.payload.lastName,
                    action.payload.date,
                ),
            ).pipe(
                map(Actions.search.success),
                catchApiError(Actions.search.failure()),
            ),
        ),
    );

Prettier 1.19.1

const searchEpic = action$ =>
    action$.pipe(
        filter(isActionOf(Actions.search.request)),
        debounceTime(500),
        switchMap(action =>
            from(
                doSearch(
                    action.payload.firstName,
                    action.payload.middleName,
                    action.payload.lastName,
                    action.payload.date,
                ),
            ).pipe(map(Actions.search.success), catchApiError(Actions.search.failure())),
        ),
    );

As can be seen, the inner pipe is formatted in a signle line while the outer pipe is formatted on multiple lines.

However, if the call to doSearch is short enough, this is the result in 1.19.1:

const searchEpic = action$ =>
    action$.pipe(
        filter(isActionOf(Actions.search.request)),
        debounceTime(500),
        switchMap(action =>
            from(doSearch(action.payload.firstName, action.payload.lastName)).pipe(
                map(Actions.search.success),
                catchApiError(Actions.search.failure()),
            ),
        ),
    );

This is formatted how I would expect. But why does the formatting of the doSearch call influence the formatting of the inner pipe?

[lydell]

Just thinking aloud: Maybe we can borrow some ideas in #6685.

[brainkim]

Right it seems like we’ve moved beyond issues of “readability” to issues of ”expectations,” as in what you “expect” prettier to produce, insofar as no one one has provided an example of “unreadable” code. As for “expectations,“ I think my inclination is just to say that your expectations are unreasonable. They were unfortunately set by the previous behavior of prettier which involved an overfit heuristic which split every call to a function or method named pipe onto multiple lines (with hacky exceptions for node.js streams). The behavior @larskinn describes is just the way prettier treats call expressions in all code, and demanding prettier to treat this code exceptionally without providing any insight into the mechanism for the underlying heuristic is uhhhhh somewhat artless.

Does the previous hard-coded naming approach seem like a good idea to you RxJS folk? Does it not strike you as a code smell? When I first realized prettier did this I was both shocked and started to wonder about prettier’s stewardship and maintenance. It’s lazy, anglo-centric, and worst of all it opens the door to the very real risk that prettier, a tool which is just supposed to be about formatting code, creates special hard-coded exceptions for Facebook or Facebook-adjacent tools and patterns to the detriment of any other library.

If you agree with me, and think that maybe a code formatter shouldn’t be determining the way call expressions are split by reading the actual name of the callee, then I encourage you to look at the code you’re saying is unexpected. Put it in an AST explorer, and describe the heuristic which prettier should use to force a split. The heuristic I ended up with was based on the insight that all of the original examples of function composition had multiple anonymous functions in them on the same line, and I made the case that this produces unreadable code insofar as each function has its own scope and adds mental overhead. It was never going to catch all examples of function composition but was a reasonable substitute, a compromise.

[thorn0]

RxJS has nothing to do with Facebook though.

[brainkim]

@thorn0 Right, but two of the hard-coded names I overwrote were connect and createSelector which are both Facebook-adjacent (redux). Also, have you ever noticed that jest test calls are always formatted onto a single line even when they extend past the line budget? 🤔 There probably was a heuristic that could’ve benefited all users but instead we decided to hard-code the test name to have that behavior.

[thorn0]

Pre-Jest names—it and describe—are hardcoded there too, as well as extra special cases for Angular's test utils. Hardcoding is a simple solution. Call it lazy, but sometimes it is good enough.

[brainkim]

It’s great until certain libraries are left in the dirt because they are deemed “not popular enough”

https://github.com/prettier/prettier/pull/1670#issuecomment-303840512

[brainkim]

“Lazy” is probably too harsh, I apologize. Hard-coding is dope af in the right circumstances. I just wanted to follow the rule of threes and needed an extra word

[merlinnot]

I honestly miss the old times when describe and it allowed their arguments to be in new lines ❤️ 💀

[thorn0]

@brainkim You have a point. While it seems obvious that heuristics are better than hardcoding, it also is something that can be forgotten and needs to be kept in mind. It was me who submitted the PR for those Angular helpers instead of thinking of a heuristic, which would've been pretty simple in that case. :(

[rassie]

@brainkim Of course I have a strong preference, but in reality, a consistent experience will still prevail (FWIW, I'm more of Perl::Tidy type of guy, just look at all those options!). My point is that putting operators on one line depending on their number is a) inconsistent and b) different in semantics from e.g. reformatting arrays or import lists.

You are probably familiar with a classic text by Joel Spolsky on training yourself to see wrong code at a glance. Tools like Prettier can contribute quite a lot to the visual part of this. For example, we are using a lot of rxjs-spy and also quite a few Subjects in our code base, so that a common pattern is something like this:

const input$ = new Subject();
const output$ = input$.pipe(
    tag('Data'),
    shareReplay(1)
)

A combination of tag and/or share/shareReplay is an essential part of our data flow. It is important that I can consistently recognize whether a stream has been tagged and whether it's being multiplexed or not. With Prettier 1.18, it's obvious: every operator is on its own line, so visual scanning is easy. With Prettier 1.19, it's dependent on number (or rather length) of operators, so that I need to watch closer. Re-ordering stuff is a matter of moving lines around with 1.18, not so much with 1.19. (maybe my point is that when I talk about "readability" I actually mean "usability", I'm quite confused by now).

Concerning exceptions for RxJS: I don't think adding code to handle edge cases better is a code smell. It's rather a testament to reality in that different language and framework communities have different needs. In my own programming, if I have to endure ugliness, I always prefer to put it in my code instead of offloading it to user experience. In case of Prettier, its aim is to cater to humans' perception, which is quite complicated as it is, so I would expect the code to have quite a bit of ugliness.

[brainkim]

My point is that putting operators on one line depending on their number is a) inconsistent and b) different in semantics from e.g. reformatting arrays or import lists.

It’s based on the presence or absence of function literals, not necessarily the number of rxjs-like operator calls (which are indistinguishable from any other nested calls btw).

Concerning exceptions for RxJS: I don't think adding code to handle edge cases better is a code smell.

So to be clear, your ask is that prettier hard-code an exception for methods named pipe for all objects? I want a clear, unambiguous statement in writing from you that this is what you want.

🔥🔥🔥 HOT TAKE WARNING 🔥🔥🔥
Just to put my cards on the table, I’m personally unsympathetic because I’ve always thought RxJS was kinda silly, and wonder how in the creation of the humongous API surface area that is RxJS operators to make observables minimally useful and replicate basic control-flow operators in javascript (takeWhile, finalize, iif), no one stopped to question if maybe this was the wrong approach. Everyone who uses RxJS admits that it‘s difficult to learn, and now we’re even asking for special formatting exceptions because of the difficulty of parsing operator pipelines. I’m looking at the docs for shareReplay and I’m still unclear on what it even does or why on Earth observables aren’t just consumed by default, especially if subscribing to them can trigger side-effects. How does this in any way resemble “functional programming”?

Thankfully, it’s up to the maintainers of prettier whether they want to accommodate this request, not me. Let me just reiterate the point I made above, which is that, I believe a tool like prettier, which is bundled with newer editors, project templates, and even runtimes like deno, should avoid even reading identifier names in code. There’s already a fast-lane for making sure prettier formats facebook-backed projects correctly (see the label "facebook: priority blocker"), and you should see how quickly the prettier team worked to accommodate the formatting of the useEffect hook. It just strikes me as unfair, and adopting a basic design decision like “prettier will not read identifier names” can do a lot to level the playing field, and prevent the complete corporate overrun of open-source libraries and mindshare in javascript.

[rassie]

So to be clear, your ask is that prettier hard-code an exception for methods named pipe for all objects? I want a clear, unambiguous statement in writing from you that this is what you want.

I don't think it matters what I want, especially since I'm currently a "vocal minority" (a RxJS user, I also don't agree with some of basic premises of Prettier). Prettier is a tool that makes my life easier and for the most part it gets its job done better than other available tools. The development team of Prettier will decide what's good and what's bad for the project's future and I will adapt, one way or another. This issue was meant as a hint from a user that some of the thinking might profit from revisiting (the original RxJS issue makes a lot of the same points), I'm not demanding rolling back anything just because I don't like it.

firefirefire HOT TAKE WARNING firefirefire
Just to put my cards on the table, I’m personally unsympathetic because I’ve always thought RxJS was kinda silly

In an earlier project I worked with Bacon.js and in both cases (Bacon then and RxJS now) it saved us from a lot of pain with complex UI interactions. It's a classic case of "you won't understand it until you need it", so I would encourage you to learn about the problems being solved there. But either way, you have to keep in mind that most of RxJS users are coming from Angular, so the user base is actually quite large.

There’s already a fast-lane for making sure prettier formats facebook-backed projects correctly (see the label "facebook: priority blocker"), and you should see how quickly the prettier team worked to accommodate the formatting of the useEffect hook.

It's not because it's Facebook, it's because these projects have a huge user base (which might be because it's Facebook). You can go back and think about JSX adoption, which also originated at Facebook and required changing parsers and generally adapting.

(it's funny, because we are talking about changes in 1.19, which literally introduces a new option for better Vue support)

It just strikes me as _unfair_, and adopting a basic design decision like “prettier will not read identifier names” can do a lot to level the playing field, and prevent the complete corporate overrun of open-source libraries and mindshare in javascript.

Have you (or would you have) opposed JSX support in Prettier? Same stuff, but some people have to work with it and from my experience autoformatting JSX is a life saver. I see no problem in big corporations releasing FOSS-licensed tools and making sure they work with the available open-source toolchain. I'm not happy about everything they do, but my daily work depends on tools and I'm more than happy to be able to use corporate-backend (React) and non-corporate-backed (RxJS, Prettier) stuff together.

[larskinn]

@brainkim

The behavior @larskinn describes is just the way prettier treats call expressions in all code, and demanding prettier to treat this code exceptionally without providing any insight into the mechanism for the underlying heuristic is uhhhhh somewhat artless.

I didn't mean to disparage your work, or that of the Prettier team in general. What mostly surprised me here was that such large parts of our codebase was formatted differently after this upgrade (we reformat the codebase in a single commit after every Prettier upgrade, so later commits won't include formatting diffs).

I agree that a heuristic is better than hard-coding names. Without insight into the implementation, the change in output was just surprising to me. Reading your explanation, it sounds like this change means formatting will be more consistent across the whole codebase, which is good.

firefirefire HOT TAKE WARNING firefirefire
Just to put my cards on the table, I’m personally unsympathetic because I’ve always thought RxJS was kinda silly

I don't see what RxJS specifically has to do with this. I feel the same way about any function pipelines (like pipe from fp-ts or ramda), and method chaining. They're often more readable when split over multiple lines in my opinion. The previous rule catered to my preference, while the new one doesn't. I'll adapt.

Overall I would prefer if Prettier could respect existing line breaks, but that ship sailed long ago.

[brainkim]

I didn't mean to disparage your work

I don’t think you were! To be clear, I love debating issues of code and think that arguing is crucial to producing good, robust systems. And prettier is this huge archive of how people think code should be formatted and there’s lots of entertaining and heated debates about how every little bit of code should be formatted. If I come across as pugilistic or defensive, I apologize. I guess part of what appeals to me about programming is the fight, the constant drive to clarify why your way of doing things is better.

What mostly surprised me here was that such large parts of our codebase was formatted differently

If you have egregious examples please do post them! I’m very willing to take a look at the examples and see if there are further refinements we can make to the heuristic.

I don't see what RxJS specifically has to do with this

The digs at RxJS are exactly as advertised, a hot take. They’re probably unrelated to the issue.

Overall I would prefer if Prettier could respect existing line breaks, but that ship sailed long ago.

I sometimes wonder what a code formatter would look like if it respected semantic linebreaks. A larger issue.

[larskinn]

I sometimes wonder what a code formatter would look like if it respected semantic linebreaks. A larger issue.

Since Prettier already takes existing line breaks into account for objects literals: I would love if that worked for function arguments (and array literals) as well.

I believe elm-format for Elm does this, although the philosophy there is entirely different from Prettier since it doesn't enforce a maximum line length.

Anyhow, thank you for your work :)

[lydell]

Since Prettier already takes existing line breaks into account for objects literals: I would love if that worked for function arguments (and array literals) as well.

We try very hard not to do more of that.

[EvgenyOrekhov]

An example with some functions from the Ramda library:

const actions = {
  toggleItem: item =>
    evolve({
      selectedItems: ifElse(
        has(item.id),
        dissoc(item.id),
        assoc(item.id, item)
      )
    })
};

Prettier 1.19 turns the above into this:

const actions = {
  toggleItem: item =>
    evolve({
      selectedItems: ifElse(has(item.id), dissoc(item.id), assoc(item.id, item))
    })
};

Which in my opinion makes it harder to read.

Update: I just realized that it worked like that before 1.19. Still, I think the issue remains.

[oscar-b]

Was also surprised by the big diff of Ramda changes:

const getMenus = pipe(
    pluck('menus'),
    flatten,
    uniqBy(prop('id')),
);

into

const getMenus = pipe(pluck('menus'), flatten, uniqBy(prop('id')));

I wouldn't mind a hardcoded list of functions (pipe and compose for our project) in our Prettier settings.

[brainkim]

https://github.com/prettier/prettier/issues/6899#issuecomment-552342543 also has more examples of code which was changed.

Before (1.18.2)

this.subjectNewLine1$
  .pipe(
    filter<string>(Boolean),
    debounceTime(300)
  )
  .subscribe((value) => {
    this.newLine1 = '';
    this.customText.lines1.push(value);
    this.updateCoverage();
    this.subjectNewLine1$.next('');
  });

After (1.19.1)

this.subjectNewLine1$.pipe(filter<string>(Boolean), debounceTime(300)).subscribe((value) => {
  this.newLine1 = '';
  this.customText.lines1.push(value);
  this.updateCoverage();
  this.subjectNewLine1$.next('');
});

In both cases printWidth was set to 120

[brainkim]

@oscar-b How do you reconcile this opinion with the fact that ramda docs have multiple instances of pipe or compose calls appearing on a single line?

[oscar-b]

@oscar-b How do you reconcile this opinion with the fact that ramda docs have multiple instances of pipe or compose calls appearing on a single line?

Isn't that perfect evidence that it needs to be configurable? Obviously, the "one action per line" is preferred by a lot of people doing functional style programming, for things like pipe/compose. The old hardcoded solution worked just the way I expected it to.

[brainkim]

Isn't that perfect evidence that it needs to be configurable?

Configurable how?

The old hardcoded solution worked just the way I expected it to.

See #4858 #4935 #5060 #5769 #5898 #5969 #6030 for example of people who do not share this opinion.

[oscar-b]

Isn't that perfect evidence that it needs to be configurable?

Configurable how?

Prettier has means of configuration, yes? Easiest would be supplying a list of function names that should be treated like in older versions. Or a setting for activating the old hardcoded list of exceptions, like functionalMode: true.

The old hardcoded solution worked just the way I expected it to.

See #4858 #4935 #5060 #5769 #5898 #5969 #6030 for example of people who do not share this opinion.

And see this ticket, and others, from people who does. Again, more evidence that this can’t be solved automagically, I’m certain you would have found a way by now.

[thorn0]

@oscar-b What's your printWidth setting? My guess is it's the main culprit of this issue, not the new heuristic.

[oscar-b]

This is our config:

{
    "printWidth": 80,
    "useTabs": true,
    "tabWidth": 4,
    "singleQuote": true,
    "trailingComma": "all",
    "jsxBracketSameLine": false,
    "quoteProps": "consistent",
    "proseWrap": "always",
}

[brainkim]

A late night thought:

If you strongly believe functional operators (basically functions which return functions) should be put on multiple lines, why not put your energies to getting the pipeline operator shipped in your preferred environment? Prettier uniformly puts the operands of pipeline expressions on multiple lines. Then you could do:

const value$ = range(0, 10)
  |> filter(x => x % 2 === 0)
  |> map(x => x + x)
  |> scan((acc, x) => acc + x, 0)
  |> toArray();

const getMenus = (arg) => arg
  |> pluck('menus')
  |> flatten
  |> uniqBy(prop('id'));

or whatever functional programming stuff you want.

If we work on a more elaborate or drastic heuristic to scratch the itch of some functional programmers, is this effort going to be obsoleted by the pipeline operator?

[thorn0]

Prettier uniformly puts the operands of pipeline expressions on multiple lines.

It prints them on a single line if they fit.

[brainkim]

@thorn0 hmmm well the heuristic would be way more straightforward. If an operand of a pipeline is a call expression, force a split.

[rassie]

@brainkim that proposal is still in stage 1 and it's very unclear which direction it will take. Typescript doesn't implement stuff until stage 3 and even if I introduced some experimental babel plugin in my environment RxJS would still need to implement so support for it which would require an update, which might be very difficult for people using Angular. I like the direction of your proposal, but, for several reasons, it's not a solution right now.

[thorn0]

RxJS would still need to implement so support for it

@rassie You're right overall, but this part is not true.

observable.pipe(operator1, operator2)

is the same thing as

operator2(operator1(observable))

which means that with the pipeline syntax it will be written as

obervable |> operator1 |> operator2

[rassie]

@thorn0 If that's so, I might try snatching that babel plugin :) Learning something new daily I suppose. Thanks!

[jrnail23]

@thorn0, even if they fit on one line, doesn't the new heuristic look for function literals?

[thorn0]

@jrnail23 not in |> pipelines (so far).

[jrnail23]

ah, ok @thorn0, I thought you were talking about pipe/compose, etc.

[ThiefMaster]

This change is making reselect selectors much less readable:

-export const isOngoingBooking = createSelector(
-  getDetailsWithRoom,
-  ({startDt, endDt}) => {
-    return moment().isBetween(startDt, endDt, 'day');
-  }
-);
+export const isOngoingBooking = createSelector(getDetailsWithRoom, ({startDt, endDt}) => {
+  return moment().isBetween(startDt, endDt, 'day');
+});

[ThiefMaster]

Here's a full diff of what updating prettier from 1.18 to 1.19 does to my (large) codebase:

https://gist.github.com/ThiefMaster/fab54b516c6eb6e2b82ddb662e71104b

I think besides the changes to simple connect(func1, func2)(Component) calls and a few unrelated things, none of the changes to createSelector calls actually improve readability - they actually make things much less readable!

The change to some jQuery $(...) calls (e.g. line 689) also isn't very pretty.

[brainkim]

@ThiefMaster Thanks for the full diff that is very helpful!
Thoughts on createSelector: in this case, I agree that readability isn’t improved. Prettier behavior now consistently does the same thing as it would if the createSelector call was an addEventListener call. One idea I have for making your code more readable is to stop breaking before concise arrow function bodies (i.e. arrow functions which don’t use () => {} curly brackets). If we treated functions with concise bodies as unbreaking, or at least breaking with lower priority, a lot of your issues with readability would be solved.

[jrnail23]

@brainkim, can you show an example of what you're thinking -- maybe a before and after view?

[ThiefMaster]

That would already improve things, but more complex selectors are likely to use curly brackets, so it would probably only work nicely there in case they have enough input to trigger the old wrapping style because they don't fit in one line.

I think this selector wouldn't be covered by your proposal for example: https://gist.github.com/ThiefMaster/fab54b516c6eb6e2b82ddb662e71104b#file-not-prettier-diff-L265 (even though that one could be rewritten to not use curly braces without making it uglier)

But it is very common for selectors with a more complex body to use curly braces and return, and there's no guarantee that each complex/big selector has enough input to trigger wrapping that way.

---

I know that adding config options is frowned upon, but maybe an option to specify functions which always get this kind of wrapping (basically what the hardcoded list that was removed in 1.19 did) wouldn't be that bad?

[brainkim]

@jrnail23
createSelector with function body brackets:

const subtotalSelector = createSelector(shopItemsSelector, items => {
  return items.reduce((acc, item) => acc + item.value, 0);
});

createSelector without function body brackets:

const subtotalSelector = createSelector(
  shopItemsSelector,
  items => items.reduce((acc, item) => acc + item.value, 0),
);

Essentially, we would use the arrow function body brackets as a hint for how to break the call.
All of the examples from redux reselect docs (https://github.com/reduxjs/reselect) use functions with concise function bodies.

@ThiefMaster

But it is very common for selectors with a more complex body to use curly braces and return, and there's no guarantee that each complex/big selector has enough input to trigger wrapping that way.

I’m asking for a little compromise. You’ve conceded that the concise body check would solve all the problems in your current codebase (with one small tweak). You’re not always gonna get the exact formatting you want. The problem is that these selectors are indistinguishable by AST from most callback calls (like EventTarget.addEventListener or EventEmitter.on), and in these cases it’s almost universally agreed that the callback should start on the arg list, and its body should only be indented once. In the case where selectors have a “more complex body,” I think the current behavior actually makes the code more readable.

[jrnail23]

@brainkim, yes, the second snippet is much better than the first, and generally reflects my personally preferred style...
The following also reflects my personal preference for such constructs:

const subtotalSelector = createSelector(
  shopItemsSelector,
  items => {
    // let's pretend there's more code here
    return items.reduce((acc, item) => acc + item.value, 0);
  }
);

[ThiefMaster]

You’ve conceded that the concise body check would solve all the problems in your current codebase (with one small tweak). You’re not always gonna get the exact formatting you want.

FWIW, I just looked for the first small case I found. Just saying that it isn't uncommon for selectors to have bigger bodies.

While I'm of course biased considering that I added the special handling for createSelector in the first place before starting to use prettier at all, I think it's objectively more readable if all selectors use the same style:

• one source arrow function or named selector function argument per line
• then another line with the function that implements the actual selector

[brainkim]

@jrnail23 Under my proposed change, that example would be formatted like an addEventListener call because the items callback has a body with curly brackets.

[jrnail23]

@ThiefMaster, I generally agree with your style points for just about any function composition.
If a function is invoked with more than one function as an arg (including arrows, function literals, or a variable pointing to a function) I put each one on a separate line.

[jrnail23]

@brainkim, perhaps a useful heuristic might be to assume any consecutive function args as indicating functional composition (and thus a new line for each).
You might further refine that heuristic by saying _all_ of the args must be functions to qualify for functional composition formatting. That way, event handlers could indeed be formatted like addEventListener rather than as is suggested above for reselect selectors, or _.pipe/compose, etc.

That being said, I have no idea how prettier's implementation works, so I'm not in a great place to tell you how things should be done. Just providing feedback from an end-user perspective 😄

[ThiefMaster]

...any consecutive function args...

I'm also not super familiar with the internals, but that sounds very messy when some oft the args are being imported. I don't think prettier resolves imports at all...

[jrnail23]

@ThiefMaster, indeed. That certainly complicates matters.

[brainkim]

@jrnail23 Yeah, these heuristics assume we can read the types of identifiers. Is foo(a, b, c) function composition or just a regular function? It would be nice to use the types of arguments to determine function composition but that’s simply not possible given prettier‘s constraints.

[jrnail23]

yeah, my kingdom for a type-aware formatting tool!

[jrnail23]

ts-prettier, anyone? 😉

[brainkim]

Copying code example from #7060 by @AdrianSkierniewski

Prettier 1.19.1
Playground link

--parser babel
--print-width 100
Input:

const capitalize = compose(
  converge(concat(), [
    compose(
      toUpper,
      head,
    ),
    tail,
  ]),
  toLower,
);

Output:

const capitalize = compose(converge(concat(), [compose(toUpper, head), tail]), toLower);

Expected behavior:
The 1.18.2 returned

const capitalize = compose(
  converge(concat(), [
    compose(
      toUpper,
      head,
    ),
    tail,
  ]),
  toLower,
);

Part of the problem IMO is the custom print width, but part of it could also be that the compose call is complicated and could be broken up because of its contents. For instance, this also looks fine, and doesn’t break up all compose calls, just the top-level one.

const capitalize = compose(
  converge(concat(), [compose(toUpper, head), tail]),
  toLower,
);

Figuring out when a call expression is “complicated enough” to warrant a break could solve a lot of the problems expressed in this issue, and has nothing to do with function composition.

@lydell

Just thinking aloud: Maybe we can borrow some ideas in #6685.

https://github.com/prettier/prettier/blob/a11836fe00aa6ab6ce02c0e0c16e9aa017cce411/src/language-js/utils.js#L888-L966

The isSimpleCallArgument function looks very interesting, and might be a more generic way to split call expressions in a way that is satisfactory for the people in this thread.

[hackel]

Here's another basic example, sorry if it's already covered above. It's frustrating that Prettier 1.19 wants to expand short, simple calls like this

this.$emit('input', this.results.map(v => v.id));
// to 4 lines:
this.$emit(
    'input',
    this.results.map(v => v.id),
);

It's triggering a lot of unnecessary changes in my codebase. We're not using any fancy functional programming libraries, just good, old Javascript.

[brainkim]

@hackel your complaint actually goes in the opposite direction of most other peoples’ in this thread. You want a call expression to be put on a single line instead of multiple whereas most of the people in this thread are complaining about a call expression being put on a single line. We could change the heuristic to require at least two call arguments to be HOF-like (a call with a function literal) before triggering the split, but this would then raise the hackles of the people in this thread. In short, it’s a delicate balancing act with no perfect solutions (that I‘ve seen so far). Lemme know if you come up with something.

[gairon]

I've got the same unexpected result with code where we're using ramda.
How about adding a new flag into config to be able to return the old behavior with functional code optionally?

[mischkl]

Here's another very simple example from our Angular + RxJS project 😕:

this.model.disabledChanges.pipe(takeUntil(this._model$.pipe(skip(1))), untilDestroyed(this)).subscribe(() => this.cd.detectChanges());

Expected:

    this.model.disabledChanges.pipe(
      takeUntil(this._model$.pipe(skip(1))),
      untilDestroyed(this)
    ).subscribe(() => this.cd.detectChanges());

[karptonite]

Now that Prettier 2.0 is out (🎉), perhaps this issue could get some new life? I don't mean to beat a dead horse, but, the changes introduced in 1.19 really do make RxJS harder to follow.

[thorn0]

Yep, we're going to look into it and do something similar to #6685 here.

[rassie]

@thorn0 Any news on this? Being unable to upgrade to newer version because of vast code changes introduced by 1.9.0 basically prevents us from using any syntax-related TypeScript 3.7+ features, would really appreciate this issue moving forward.

[thorn0]

@rassie Could you help with a PR probably? There is a logic that determines whether call chains should be split onto multiple lines based on whether the arguments are considered "simple" (isSimpleCallArgument). The idea is to apply the same logic to this problem.

[ThiefMaster]

I'm still unsure about whether a heuristic is the way to go in the end... I know the project doesn't want to add many configuration options, but maybe this is one of the things that should be configurable?

Especially for the case of reselect selectors: It really makes the code less pretty and readable if different selectors get different formatting based on some heuristic. Determining the behavior based on a function name (e.g. createSelector) results in a much more consistent formatting.

[thorn0]

@ThiefMaster Hardcoding function names for specific libraries falls under the umbrella of #4424.

[rassie]

@thorn0 I don't think I comprehend all edge cases fully to craft a proper PR, I've tried applying my naive mental approach on examples from this thread and failed immediately.

On a global scale, I think #4424 might be the way to go (since every language/library/framework community has their own ideas about canonical formatting) but sadly it doesn't look to be very far by any means.

[thorn0]

Two interesting features that the examples of functional composition calls in this issue have in common:

• They never are expression statements.
• Their callee is usually a simple identifier.

[karptonite]

Two interesting features that the examples of functional composition calls in this issue have in common:

• They never are expression statements.
• Their callee is usually a simple identifier.

I'd say you definitely can't count on the second feature. A common pattern in ngrx is something like:

store.select(selector).pipe(
  map(foo => bar),
  filter(bar => baz)
);

In general, I think people in this thread have provided the simplest examples that display the unwanted formatting, not necessarily their typical use cases.

[thorn0]

@karptonite Your example is formatted as expected. We're discussing cases that aren't caught by the current solution.

[thorn0]

Actually I'm interested to see a counter-example: a non-statement call whose callee is a simple identifier and whose last argument is a Node-style callback.

[distante]

Does anyone knows which exactly is the commit where this change was introduce it? I would like to know what exactly changed.

[brainkim]

@distante you're looking for these commits https://github.com/prettier/prettier/pull/6033/commits

[distante]

@distante you're looking for these commits https://github.com/prettier/prettier/pull/6033/commits

Thanks @brainkim

[distante]

Hi @rassie, maybe can you update the title to clarify that it was introduced in 1.19 but it is still present in 2.x ?

[alizbazar]

Here's one more example that's driving us mad. Seems highly related.

Original formatting:

const getEntryIDsInResponse = flow(
  get('sections'),
  values,
  map(
    flow(
      get('entries'),
      keys
    )
  ),
  flatten
)

...gets formatted to:

const getEntryIDsInResponse = flow(get('sections'), values, map(flow(get('entries'), keys)), flatten)

This is totally unreadable. :/ Would love any tips on how to solve this.

[lydell]

Would love any tips on how to solve this.

Here’s a tip! Set printWidth to something 100 or less. See https://prettier.io/docs/en/options.html#print-width for more information.

Prettier 2.0.5
Playground link

--parser babel

Input:

const getEntryIDsInResponse = flow(
  get('sections'),
  values,
  map(
    flow(
      get('entries'),
      keys
    )
  ),
  flatten
)

Output:

const getEntryIDsInResponse = flow(
  get("sections"),
  values,
  map(flow(get("entries"), keys)),
  flatten
);

[alizbazar]

Yeah... You still have a creepy map(flow(get("entries"), keys)).

For now I added a comment in between the lines, which seems to prevent joining them. But of course that's not really optimal...

[darsain]

On the other hand here is my simple straightforward single-line keybinding hook:

useKey(' ', useCallback(() => enableShortcuts && toggle(), [enableShortcuts]));

That gets broken up into these redundant 4 lines:

useKey(
  " ",
  useCallback(() => enableShortcuts && toggle(), [enableShortcuts])
);

It looks bad enough to make me end these lines with // prettier-ignore :(