Rxjs: Automatic code formatting via prettier

[joshribakoff]

Feature Request

https://prettier.io/

Is your feature request related to a problem? Please describe.

Code formatting is manual which is tedious, it places increased cognitive load on the developer because formatting is not very consistent. Simply inserting white space causes red errors in your editor, which could easily be auto-fixed.

Describe the solution you'd like

Add prettier https://prettier.io/

Describe alternatives you've considered

n/a

(If this is new operator request) describe reason it should be core operator

n/a

Additional context

You have a few options:

• Rip the band aid off. Apply it to whole project, merge master in open PRs at the commit prior to applying it, then merge in the application of prettier to open PRs with strategy --theirs. Conflicts due to formatting will not occur as part of the conversion, and Prettier will tend to decrease formatting related PR friction going forward.
• Use a tool that incrementally applies the formatting only to lines as they are changed, this avoids the rip the band aid off approach, however this makes things worse for a long time before they become better. There will be increased merge conflicts / PR churn for a long time.

Also consider:

You use formatting that isn't represented in the AST, eg indenting lines to visually align for marble tests. In these cases, we would lose some manual formatting (but keep in mind this manually formatting can really irk some devs). You'd probably need to fix some marble tests to put the whitespace inside the string so the alignment tokens are part of the AST nodes.

This would actually make things more approachable. Most projects today written in JS/TS are using Prettier, and the maintainers of RxJs are creating tools, like Marble tests, which are less approachable if you're not manually formatting your code base like the RxJs maintainers are.

[kwonoj]

This is already attempted https://github.com/ReactiveX/rxjs/pull/3405 and at the moment we blocked by upstream https://github.com/prettier/prettier/issues/4172. If our blocker's resolved (seems so) we may retry.

[joshribakoff]

Why is this a blocker? If a pipe is very short its fine to keep in one line. In fact RxJS has single line pipelines all over the place.

I use Prettier with RxJs, and it works fine. This should not be a blocker, IMO. This feels like you're just trying to hang onto old ideas about manually formatting instead of "just going with the flow" with Prettier. I doubt they will ever change this, and I actually agree with the way it works now.

[kwonoj]

As you may see in issue itself, lot of ppl have different opinion for this formatting and prtiier's opinionated way won't fit for some. It may working it for you, but we weren't in favor.

[joshribakoff]

Prettier would at least be a standard formatting that not everyone likes, instead of your own conventions that not everyone likes. This stance feels pretty stubborn, with all due respect.

• furthermore the density of your current formatting choices are just downright not accessible to some devs with vision/reading disability, in my opinion.

[kwonoj]

If we opt in any auto formatter it'll be prettier, yes. I'm just saying there were some debate on our styling to what prettier does - it's style anyway, there's no single answer about specific preferences and prettier always explicitly mentioning its opinionated, above blocker was we have a different opinion to it.

Prettier is widely used, but not all of js / ts project is using it. it maybe standard for any autoformatter if it's being used, but not standard for all code formatting. My work also uses prettier, I am neutral about this, I just wanted to point out there were debate to introduce it into our codebase.

[joshribakoff]

Cool thanks for clarifying, to be clear if there is some better tool than Prettier I am all for that.

Right now the code is pretty dense to me, and I have RSI, so manually formatting literally is painful. The project could be more accessible, which is one of the reasons I opened https://github.com/ReactiveX/rxjs/issues/5536 for an "out of the box" watch command for unit tests.

'm just saying there were some debate on our styling to what prettier does - it's style anyway, there's no single answer about specific preferences and prettier always explicitly mentioning its opinionated,

ya Prettier isn't perfect. But if I write .pipe(tap(foo), map(bar)) I think that's fine, we have way worse in the code now w/ the manual formatting. Prettier will actually apply a heuristic. Prettier is the result of considering input from a very large dev community, way larger than the RxJs core team, so Prettiers choices are more representative of the dev community at large & more people who could potentially become contributors may find the code more approachable.

With Prettier, the RxJs core team need not waste cycles syncing about code formatting, its one less thing to worry / debate about. Also I want to point out, right now your contributor guide says something to the effect of "just try to follow the existing formatting, we'll define it better later".

[kwonoj]

I'll leave let other member chime in for other opinions since I'm neutral and I believe I shared histories enough.

Just one thing,

I think that's fine,

I think that's crux of all styling debat, isn't it? 😅 Some ppl might think some style's not right for them or not. What you mentioned is based on the assumption prettier's style will lower the bar to readability, which I mostly agree though.

[joshribakoff]

Some ppl might think some style's not right for them or not

Yep, totally, Prettier will not give a style everyone agrees on, because no such thing exists :) I'm glad you agree. Just to drive the point home, the reason I think it will improve things is that overall, it will make decisions that more people find as acceptable style. When I pushed for Prettier at Twitch for ~300 devs in a mono-repo, there was a lot of objection. There were a lot of complaints, but the complaints were far overshadowed by the praise, and the rumblings mostly went away when people started to learn most of the time Prettier does something odd, its due to code that should be refactored.

For example consider:

a$.pipe(buffer(b$.pipe(switchMap(createC$()))))

This code is bad not because of formatting, but because it mixes multiple streams at different levels of abstraction in one statement.

Instead of tweaking formatting, I'd change to:

const myClosingNotify = (b$) => b$.pipe(switchMap(createC$()))
const myBuffer = (b$) => buffer(myClosingNotify(b$))
a$.pipe(myBuffer(b$))

The real issue was the mega observable anti-pattern, not the formatting

[cartant]

Why is this a blocker?

It's simple. This will happen and the blocker is the marble tests. They all need to be re-written using TestScheduler#run so that aligning whitespace can be moved inside the marble string literals. ATM, in non-TestScheduler#run tests, whitespace in the marbles is meaningful so it's outside and formatting with Prettier messes up the alignment making the tests illegible.

[joshribakoff]

@cartant the issue that was being referred to as the "blocker" here was unrelated to what you're discussing

[cartant]

That issue - AFACIT - is stylistic. Even if that's been resolved - it's two years old - this will be blocked on the tests being changed to use run mode - see https://github.com/ReactiveX/rxjs/pull/4988#issue-311640594. And if it hasn't been resolved, opinions have probably changed in two years anyway.

The tests being reformatted as unintelligible is more significant than a style issue.

You're welcome to help expedite this by contributing PRs that convert the tests.

[joshribakoff]

I'd be glad to do all the work required here, and I will dedicate time to see it through, address any issues people open regarding the conversion, etc :)

I'm well aware the marble alignment will be changed, I called that out in the OP, thanks for calling it out again, let me clarify I'd be more than happy to handle all of this work.

However, I want to avoid doing all of this work only to have the PR rejected because of a stylistic disagreement with the way prettier's heuristic works.

I think Prettier would be a great addition that will make RxJs way less scary for newcomers when they try to read the source code, if the team is ok with it, I look forward to making it happen :)

[cartant]

FWIW, curiosity got me looking at that Prettier issue. It seems that a PR that added special handling for a number of functions/methods - https://github.com/prettier/prettier/pull/4431 - was opened and merged. However, it seems that behaviour was later reverted. Prettier now formats on a single line - if there is sufficient space available - and only leaves the arguments on separate lines if there is a comment:

Prettier Playground example

IMO, Prettier can do whatever it wants with the formatting. The advantages of using it outweigh the disadvantages, as far as I'm concerned.

[rassie]

Prettier has changed their heuristic with 1.19, which (would have) reformatted a large chunk of our code to an incoherent mess. Details in https://github.com/prettier/prettier/issues/6921 and until that ticket is resolved, I wouldn't try to upgrade from 1.18. I assume the same would be true of RxJS project, it's a bad time to start with Prettier, unless you explicitely use 1.18.

[benlesh]

Moving to Prettier has started with #5552

We can close this.