Never inline decorators, unless they're lone parameter decorators by suchipi · Pull Request #4830 · prettier/prettier

suchipi · 2018-07-10T19:16:43Z

cc @vjeux who might have more context than me on this.

suchipi · 2018-07-10T19:18:09Z

tests/typescript_decorators/__snapshots__/jsfmt.spec.js.snap

-  constructor(@ILifecycleService lifecycleService) {}
+  constructor(
+    @ILifecycleService
+    lifecycleService


What does this syntax mean?

Here’s what TS has to say:

The expression for the parameter decorator will be called as a function at runtime, with the following three arguments:

Either the constructor function of the class for a static member, or the prototype of the class for an instance member.

The name of the member.

The ordinal index of the parameter in the function’s parameter list.

Interesting. Their example makes it a bit clearer:

class Greeter { greeting: string; constructor(message: string) { this.greeting = message; } @validate greet(@required name: string) { return "Hello " + name + ", " + this.greeting; } }

We should keep these inline.

suchipi · 2018-07-10T19:19:00Z

tests/decorators/__snapshots__/jsfmt.spec.js.snap

-  @observable amount: number = 1;
+  @observable
+  price: number = 0;
+  @observable


It felt a bit odd to not have an empty line between these, but since we preserve user-entered empty lines, I think it's fine (see also the type orm snapshot)

suchipi · 2018-07-10T19:19:25Z

src/language-js/printer-estree.js

-      ? parent === firstNonConditionalParent
-        ? group(doc)
-        : doc
+      ? parent === firstNonConditionalParent ? group(doc) : doc


My bad, my editor formatted with an older version of prettier

suchipi · 2018-07-10T19:19:29Z

src/language-js/printer-estree.js

-              : hasComment && !isOpeningFragment
-                ? " "
-                : "",
+              : hasComment && !isOpeningFragment ? " " : "",


vjeux · 2018-07-10T20:14:11Z

Originally I put all of them in their own line but there were a lot of people using mobx which complained. Apparently it’s the convention over there to inline them.

I have no opinions myself.

egeozcan · 2018-07-11T16:08:20Z

@vjeux mobx is the only library which inlines them as a convention. even in the spec itself, they are on their own line. I couldn't find any language which has something similar to decorators and inlined them by convention. See the last comments on #2613

kamilmysliwiec · 2018-07-13T12:02:42Z

Fully agree. Inline decorators look awful and it's quite a standard to keep them in the separated lines. TypeOrm entities, as well as NestJS/Angular stuff, looks really bad once they're inlined. Actually, I went here because I was pretty sure that this issue is finally sorted out.

suchipi · 2018-07-17T21:16:25Z

I'm hoping to get this into the 1.14 release, but I won't have time to work on this until Thursday evening. I'll add it to the milestone for now, but other maintainers, remove it if you need to (or finish the work for me 😝).

suchipi · 2018-07-20T03:04:03Z

Ok, this is ready for review/merge

duailibe · 2018-07-20T13:49:50Z

src/common/util.js

+  if (
+    !(
+      parent[propertyName] &&
+      Object.prototype.toString.call(parent[propertyName]) === "[object Array]"


You can use Array.isArray (we use it in several places)

Oh nice! I'll change it.

Array.isArray isn’t supported on Node 4; do we polyfill?

@j-f1 it is supported. On node 4, it doesn't work with Proxys or subclasses

connorjclark · 2018-07-23T20:34:35Z

I'd like to thank the project maintainers for listening to the community on this issue. Thank you for making a great tool!

devuxer · 2018-08-17T01:16:42Z

I just noticed this change when formatting a file in a mobx project.

I get the motivation for it, so I don't disagree with putting decorators on a separate line, but I think it would be more readable to put an extra line break between decorated properties.

Harder to read

@observable
public name = "";
@observable
public location = "";
@observable
public nationId = "";

Easier to read

@observable
public name = "";

@observable
public location = "";

@observable
public nationId = "";

Agree/disagree? I can make this a new issue if people agree.

lydell · 2018-08-17T05:48:17Z

So far, Prettier has never inserted blank lines into your code, as far as I know (only removed/collapsed blank lines). So this would be the first case where it inserts them. Might be good, might be confusing. Not sure. Anyway, check out the discussion in #4924 first.

devuxer · 2018-08-17T07:41:01Z

@lydell,

Very interesting. I guess the premise was incorrect that mobx was the only thing using inline decorators, so this is really quite a tough one to solve. I put a suggestion in #4924 as well.

Rob1kelly15 · 2018-08-24T19:53:26Z

Is there a way to configure this to turn off? We work in Angular 6, and when it comes to our input and output decorators, we want them inline, but this prevents us from doing so, since we have it to auto format on save

j-f1 · 2018-08-24T20:12:27Z

Please discuss this over in #4924.

suchipi commented Jul 10, 2018

View reviewed changes

suchipi added the status:wip Pull requests that are a work in progress and should not be merged label Jul 10, 2018

suchipi added this to the 1.14 milestone Jul 17, 2018

suchipi added 2 commits July 19, 2018 21:02

Add TypeScript decorator tests

bf473af

Always use hardline for decorators, unless they're parameter decorators

c458d34

suchipi force-pushed the dont_inline_decorators branch from 90c7b9e to c458d34 Compare July 20, 2018 03:03

suchipi removed the status:wip Pull requests that are a work in progress and should not be merged label Jul 20, 2018

suchipi changed the title ~~Never inline decorators~~ Never inline decorators, unless they're parameter decorators Jul 20, 2018

suchipi changed the title ~~Never inline decorators, unless they're parameter decorators~~ Never inline decorators, unless they're lone parameter decorators Jul 20, 2018

duailibe reviewed Jul 20, 2018

View reviewed changes

duailibe approved these changes Jul 20, 2018

View reviewed changes

Use Array.isArray

31f47c0

suchipi merged commit 3bfaf66 into prettier:master Jul 20, 2018

Slessi mentioned this pull request Aug 1, 2018

New release? (1.14) prettier/prettier-vscode#533

Closed

bengry mentioned this pull request Aug 1, 2018

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

Closed

prettier locked and limited conversation to collaborators Aug 24, 2018

Uh oh!

Conversation

suchipi commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suchipi Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Jul 10, 2018

Uh oh!

egeozcan commented Jul 11, 2018

Uh oh!

kamilmysliwiec commented Jul 13, 2018

Uh oh!

suchipi commented Jul 17, 2018

Uh oh!

suchipi commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duailibe Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Jul 23, 2018

Uh oh!

devuxer commented Aug 17, 2018

Uh oh!

lydell commented Aug 17, 2018

Uh oh!

devuxer commented Aug 17, 2018

Uh oh!

Rob1kelly15 commented Aug 24, 2018

Uh oh!

j-f1 commented Aug 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

suchipi Jul 10, 2018 •

edited

Loading

duailibe Jul 20, 2018 •

edited

Loading