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

Angular Output/Input decorators are moved to new line #4924

Closed
bengry opened this Issue Aug 1, 2018 · 109 comments

Comments

Projects
None yet
@bengry

bengry commented Aug 1, 2018

Prettier 1.14.0
Playground link

--parser typescript

Input:

@Component({
  selector: 'toh-hero-button',
  template: `<button>{{label}}</button>`
})
export class HeroButtonComponent {
  @Output() change = new EventEmitter<any>();
  @Input() label: string;
}

Output:

@Component({
  selector: "toh-hero-button",
  template: `<button>{{label}}</button>`
})
export class HeroButtonComponent {
  @Output()
  change = new EventEmitter<any>();
  @Input()
  label: string;
}

Expected behavior:
Decorators are kept on the same line.

More details:
When using Angular, it's recommended to follow their style guide, which specifies:

Placing the decorator on the same line usually makes for shorter code and still easily identifies the property as an input or output. Put it on the line above when doing so is clearly more readable.

Angular classes are a valid (and common) use-case for Prettier, but with Prettier 1.14 (relevant PR) it deviates from the Angular style guide, causing unwanted changed for most people using Angular.
Note that the above code example (input) is taken directly from the Angular style guide.

The original PR to change this behavior mentions that mobx is the only library using same-line decorators, which is clearly not the case, with Angular being a (big) second one.

To avoid reverting the new behavior completely, which is probably still wanted by some people - I suggest adding an option to the config to keep the old behavior, or prefer the new one (default should be discussed further).

@babeal

This comment has been minimized.

babeal commented Aug 2, 2018

Please no, we just fought so hard for this. Decorators on the same line are absolutely horrible to read and I read a lot of code. I need to be able to quickly find variables and their assignments and a mix ruins the flow. Why angular ever recommended inline decorators is beyond me. And while it is a recommendation with both sets of language it does not deviate. The last line “put it on the line above when doing so is clearly more readable” is, in a lot of our opinions, always true. I absolutely can’t stand inline decorators about as much as I can’t stand constructor accessibility modifiers.

@zh99998

This comment has been minimized.

zh99998 commented Aug 2, 2018

Please no, don't add more unnecessary options.

@egeozcan

This comment has been minimized.

egeozcan commented Aug 2, 2018

Decorators/attributes are always in their own line in all other languages + the decorator spec itself. No need to start a new trend here.

#2613 (comment)

Angular and MobX (btw mobx is a library I love, but regardless) maintainers should not come up with incompatible style decisions.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 2, 2018

In Angular, property decorators look and feel like modifiers. Like private, static, readonly, etc., only custom. Having @Input() on a separate line is as unusual as having private on a separate line.

@scott-ho

This comment has been minimized.

scott-ho commented Aug 3, 2018

@johnpapa @IgorMinar @github/angular What's your opinion?

@egeozcan

This comment has been minimized.

egeozcan commented Aug 3, 2018

@thorn0 why don't they use the same functionality from typescript? IMHO, it doesn't make sense to inline them just because there is a single specific (and, again IMHO, hacky[1]) use-case which imitates some usually-inline syntax.

[1] I fully respect the developers who came with this. They most probably had their reasons. This doesn't however affect my point.

@suchipi

This comment has been minimized.

Member

suchipi commented Aug 3, 2018

We're not going to add an across-the-board option for this; see prior discussion in #2613 and #325.

We might be open to a solution that meets Angular users' needs as long as it doesn't cause inlining for everyone else. But I don't think detecting @Input() and @Output() is safe, because those could be used in other libraries- we've recently gotten a little flak for detecting pipe(...) as a function programming composer function when it's often something else.

Also, I feel we are starting to go down a slippery slope of special-casing things for certain libraries. It's not maintainable long-term.

@suchipi suchipi changed the title from Add option for specifying decorator placement on same/different line to Angular Output/Input decorators are moved to new line Aug 3, 2018

@bengry

This comment has been minimized.

bengry commented Aug 3, 2018

@suchipi I'm not suggesting detecting anything automatically, but rather have an option in the config file to either give decorators their own line, or keep them inlined (for class properties at least).

This is not ideal, since it further expands the config options that prettier has, which should be minimal by definition, with it being opinionated. However, I think not having it (or another solution) removes a good portion of potential users from using Prettier, for a rather small issue that could be solved.

@j-f1

This comment has been minimized.

Member

j-f1 commented Aug 3, 2018

@bengry: From @suchipi’s comment:

We're not going to add an across-the-board option for this; see prior discussion in #2613 and #325.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

@egeozcan You misread my comment. I didn't say they have the same semantics as private, etc. However, just like private, property decorators are little markers that don't deserve separate lines.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

If a property decorator's length (with its arguments) is long enough (>15 characters, probably), then of course they don't look like little modifiers any more and need a separate line.

@j-f1

This comment has been minimized.

Member

j-f1 commented Aug 3, 2018

How about if inlining the decorator is named either /[A-Z][a-z0-9]+/ or /[a-z0-9]+/ (i.e. Foo or foo but not FooBar)?

@suchipi

This comment has been minimized.

Member

suchipi commented Aug 3, 2018

That seems like it would surprise users...

@egeozcan

This comment has been minimized.

egeozcan commented Aug 3, 2018

@thorn0

Would you like something like this to be common in JS:

class Test {
    @IsNotToBeInlined
    label: string;
    @IsToBeInlined otherLabel: string;
}

...while literally every language ever invented a) has all its style guides having them on their own line or b) doesn't even allow them inline?

private, protected and other access modifiers are not user definable and it is common to group properties with common modifiers together to have the names aligned.

On the other hand, attributes can be as long (or short) as the developers want. This is the main reason why they get their own line. Sorting them by length to improve the roller-coaster reading experience may not make logical sense. If you start inlining them, it is proved to be impossible (by many style-guide authors, as evidenced by their mail lists and the guides themselves) to find a logical point where it's ok for them to be inlined.

If you are only using them with Angular, sure. This may seem like a lot of bike-shedding. Yet, try using them in bigger projects with many decorators coming from various sources. In this case, should Angular coding style be changed when it's used with, say, TypeORM?

TL;DR: We should learn from other communities, and not re-invent things.

@babeal

This comment has been minimized.

babeal commented Aug 3, 2018

^agreed and even the missing line space between decorator and previous declaration is irking me. Please keep decorators on separate line to have one consistent pattern for decorators.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

it is common to group properties with common modifiers together to have the names aligned.

And guess what, people do exactly same thing with decorators.

@egeozcan

This comment has been minimized.

egeozcan commented Aug 3, 2018

In addition: In Python, they tried to allow more decorators per line (not even the same line as the property) in 2.4a2. They reverted that on a3 and finalized on 2.4 stable that every one of them gets a new line because otherwise it was too inconsistent. I remember the discussion in the mail lists.

Please just look at all the decorator/attribute/whatever-it-is-called examples from all the languages. It is like that for a reason.

And guess what, people do exactly same thing with decorators

Respectfully, I suggest you to read the whole argument around the part you quoted.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

Working with Angular, I totally like the behavior of Prettier 1.13: 1) if a property declaration is too long to fit on one line, the decorator is the first thing that is moved to a separate line; 2) if there is more than 1 decorator, they get separate lines.

@egeozcan

This comment has been minimized.

egeozcan commented Aug 3, 2018

Quoting myself here, maybe you have missed it:

If you are only using them with Angular, sure. This may seem like a lot of bike-shedding. Yet, try using them in bigger projects with many decorators coming from various sources. In this case, should Angular coding style be changed when it's used with, say, TypeORM?

@suchipi

This comment has been minimized.

Member

suchipi commented Aug 3, 2018

@egeozcan I don't think there's much more about opinion to be added to the discussion here. Don't worry, we won't revert #4830.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

Looking at example code for TypeORM, I noticed something interesting:

@Entity()
export class PhotoMetadata {
    @PrimaryGeneratedColumn()
    id: number;

    @Column("int")
    height: number;

    @Column("int")
    width: number;

    @Column()
    orientation: string;

    @Column()
    compressed: boolean;

    /// ...
}

They had to add blank lines between properties, otherwise it becomes barely readable:

@Entity()
export class PhotoMetadata {
    @PrimaryGeneratedColumn()
    id: number;
    @Column("int")
    height: number;
    @Column("int")
    width: number;
    @Column()
    orientation: string;
    @Column()
    compressed: boolean;
    /// ...
}

Which basically means that with Prettier 1.14, every property that used to need only one line before, now needs 3.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

Prettier should add those blank lines too then. Without them, it's a mess.

@babeal

This comment has been minimized.

babeal commented Aug 3, 2018

^absolutely. But my understanding was that prettier honored blank lines that users add. So if you have them bunched up they will rendered bunched but do you add the space it will be honored. Is that not the case?

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

It is the case of course. But imagine Prettier 1.14 reformatting a big Angular or MobX project with decorators inlined and no blank lines between properties. The output won't be pretty.

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 3, 2018

@egeozcan For the record, what you wrote about "literally every language ever invented" is a pretty bold statement. At least, in the C# world it's not a universal truth. People use attributes in many different ways. E.g. see dotnet/roslyn#11572

@egeozcan

This comment has been minimized.

egeozcan commented Aug 3, 2018

@thorn0 being alowed by the compiler does not mean it's in any style guide. ReSharper and VS automatically move them on their own lines. I said (please note the "or"):

...while literally every language ever invented a) has all its style guides having them on their own line or b) doesn't even allow them inline?

@suchipi sorry for the noise :) and thanks for the clarification. I'm out.

@dmarg

This comment has been minimized.

dmarg commented Aug 30, 2018

With all that being said there definitely should be a new line inserted after if Prettier converts a single to multiline to separate the decorators.

@karptonite

This comment has been minimized.

karptonite commented Aug 30, 2018

@vjeux This issue has been marked "needs discussion". I suggest that it has now had plenty, and it is time to move forward. I'm tagging you because you have the authority to make a call here--sorry if I'm overstepping.

Based on the discussion, it seems that in the current state, mobx and Angular developers will have to lock to 1.13. A few path forward are summarized above by @devuxer.

Are any of these options that make enough sense that we can settle on one, and someone can start on a pull request?

@babeal

This comment has been minimized.

babeal commented Aug 30, 2018

I am speaking for all the people that voted to have it changed this way, which is documented in the previous issue. They aren’t here since it works the way they want. I’m not trying to be rude in anyway, just want to provide representation :). I would be fine with an option as it is a preference.

In your first example, when you add the public accessibility modifier, does that make a difference for you? I spend most of my time daily code reviewing over many different languages and implicit accessibility modifiers causes extra time when I read the line to remind myself what the default is. It’s minuscule, but it adds up throughout the day so I prefer elements like this to be explicit. Once the line gets extended it just adds to the cognitive delay. It is completely subjective, like my inability to use dark ui’s when reading code. Speed in recognition and interpretation is extremely important in what I do daily so every little bit counts.

Though prettier isn’t perfect for this as it really messes up long generics and rxjs observables but at least it’s better than some of the special layouts our developers have cooked up.

@dmarg

This comment has been minimized.

dmarg commented Aug 30, 2018

@karptonite I'm not using Angular nor mobx and I tried downgrading to prettier 1.13 and it still did the multiline on all of the decorators.

Again, I feel like the option would be the best solution and don't understand why some are against adding the option.

@karptonite

This comment has been minimized.

karptonite commented Aug 30, 2018

@babeal since you are speaking for the users who voted to have it this way, perhaps you can address the fact that the current state goes contrary to the decorator proposal, here:
https://github.com/tc39/proposal-decorators#decorators
Note that they show property decorators (only) inlined.

@dmarg as for why some are against adding an option, prettier is by design an opinionated formatter, and seems only to offer options in rare cases where the community is so divided that there seems to be no other, uh... option. I'm not sure why downgrading to 1.13 didn't cause your decorators to be inlined; I'm locked in to 1.13.7, and my decorators are definitely inlined.

@dmarg

This comment has been minimized.

dmarg commented Aug 30, 2018

@karptonite I was using it on a TS file and it did not inline the decorators... just removed prettier and then reinstalled it and locked it again to 1.13.7 and it appears that it is ad hearing to the TC39 proposal which is what I like.

I do think there should be an option. I hear you on that prettier is opinionated and we want to reserve options for contentious items, but I feel that this maybe one of those times to use an option.

@karptonite

This comment has been minimized.

karptonite commented Aug 30, 2018

I'm actually neutral on whether we should add an option, as long as decorators can be inlined for properties. The reason I'm suggesting option 2 as a formatting that should make most users happy is that the prettier team leans strongly against options, and I don't want to give the impression that we are saying "it's an option or nothing" and have the response be, "then nothing." I would argue that inlining property decorators should be the default (in line with the TC39 proposal), and that those who want them on a separate line should be requesting an option.

@babeal

This comment has been minimized.

babeal commented Aug 30, 2018

@karptonite just because the syntax of the language supports it doesn’t mean it’s good code which is why we have tools like tslint, resharper, fxcop, etc. There is nothing in the proposal that suggests any recommended style.

@suchipi

This comment has been minimized.

Member

suchipi commented Aug 30, 2018

I'm gonna lock this and limit discussion to contributors. I don't know what the solution will be but I think we've covered a lot of ground at this point and we understand the problem space and the limitations of each potential solution.

@prettier prettier locked as too heated and limited conversation to collaborators Aug 30, 2018

@lydell lydell added the type:bug label Aug 31, 2018

@lydell

This comment has been minimized.

Collaborator

lydell commented Aug 31, 2018

Adding the “bug” label to make it more clear that we intend to improve this (but haven’t decided exactly how yet).

@lydell

This comment has been minimized.

Collaborator

lydell commented Sep 29, 2018

This issue has now been open for two months, along with #1974 and #5088. We have found neither a good heuristic nor options that would work for all use cases.

Prettier keeps object literals multiline if they were written that way due to all the different things objects are used for, and we couldn't find a good heuristic for all cases. Looks like decorators are used for a lot of different things too.

After some internal discussion it was decided to take the same approach with decorators as with objects. While this is not ideal, we need to unblock this (while still being open for better solutions in the future).

We're open to a PR implementing the following:

  • If all decorators for a thing (property, function, class, etc) are written inline:
    • If they fit, keep them that way.
    • Otherwise go multiline.
  • If at least one decorator for a thing is on its own line (or spans multiple lines itself), go multiline (even if they would fit inline).
  • In other words, decorators will be either all on separate lines, or all inline.
  • Note that the decorators for one property of a class can be multiline, while another property on the same class can be singleline.
  • There might be some edge case I didn't think of, but we'll find out when a PR is made.

Hope I didn't miss anything.

@lydell lydell added the help wanted label Sep 29, 2018

@prettier prettier unlocked this conversation Sep 29, 2018

@devuxer

This comment has been minimized.

devuxer commented Sep 29, 2018

I agree with the following:

  • If all decorators for a thing are written inline:

    • If they fit, keep them that way.
    • Otherwise go multiline.
  • If at least one decorator for a thing is on its own line (or spans multiple lines itself), go multiline (even if they would fit inline).

  • In other words, decorators will be either all on separate lines, or all inline.

  • There might be some edge case I didn't think of, but we'll find out when a PR is made.

But I would add one more:

  • If one or more decorators is on a separate line from the thing, ensure there is at least one line break before the first decorator and at least one line break after the thing.

Example:

// before
@decorator1
thing1
@decorator2
thing2
@decorator3
thing3

// after
@decorator1
thing1

@decorator2
thing2

@decorator3
thing3
@lydell

This comment has been minimized.

Collaborator

lydell commented Sep 29, 2018

I see what you mean, but let's wait doing the blank lines stuff for a later PR to keep things moving.

@devuxer

This comment has been minimized.

devuxer commented Sep 29, 2018

Fair enough. Should a separate issue be created for this feature?

@lydell

This comment has been minimized.

Collaborator

lydell commented Sep 29, 2018

Let's wait until the decorator placement is done. See also #4830 (comment) and #4830 (comment).

@devuxer

This comment has been minimized.

devuxer commented Sep 29, 2018

Yep, I've seen those comments, but I still feel the same about this feature. I'll stand by until your proposal is done, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment