Nuclide run-through about prettier

[vjeux]

Omg, you are on a streak <3

[vjeux]

<3

[jlongster]

(Sigh accidentally posted, editing it to finish)

All of these look really good! None of them changes the few things I'm opinionated about, so I'm really happy to work with you on this. Most of these are pretty easy. Also, I think we should have CLI options to configure a few of them (singe/double-quotes and spaces inside objects/arrays at least). That will help adoption.

Prioritize breaking on ? and :

It will be interesting to figure this out because the way it formats it now is because of how the AST is structured, which is based on operator precedence. If we force it to be different, we'll probably run into other weird formatting when the ternary operator is involved in a longer expression. We should create a test file with a bunch of different types of expressions involving a ternary.

I have no problems changing this though, hopefully it's not too hard.

I don't think it's possible to automatically add empty lines in the right place most of the time. I think this should be under the developer control to decide where to put them.

Yeah, I think you're right. FWIW refmt does not do this, and I was trying to be as opinionated as it. However, I think this would be another thing that would help adoption.

I think we could at least collapse multiple empty lines into a single empty line. Otherwise, keep single and double newlines in the output, which allows the programmer to group code.

Invalid code generated

That's not invalid! :) I'm kidding, I know what you mean. It moved the comments up. Comments are hard, and that behavior is from recast, because it doesn't know if the comment is attached to the
current line or the next node. There are a few issues on recast about comments, and this comment (benjamn/recast#191 (comment)) explains the reason for this exact behavior.

Because we've forked it though, we can see if there's a small fix that works for the majority of the cases.

Remove trailing whitespace

Yep, I introduced this last night with an initial attempt to add blank lines. I thought it's tricky to insert only blank lines (because when you insert a line, it automatically indents it) but I just realized we can easily solve this in the code that actually prints the final string by forcibly removing any spaces on blank lines.

Implement call chains

This might be the hardest to implement out of all of these requests. I like the idea and we'll see what we can do. Traversing the AST and printing CallExpressions more manually might work.

[vjeux]

Cool! For collapsing more than a single empty line, I'm onboard :)

…

On Dec 31, 2016, at 5:36 PM, James Long ***@***.***> wrote: All of these look really good! None of them changes the few things I'm opinionated about, so I'm really happy to work with you on this. Most of these are pretty easy. Also, I think we should have CLI options to configure a few of them (singe/double-quotes and spaces inside objects/arrays at least). That will help adoption. Prioritize breaking on ? and : It will be interesting to figure this out because the way it formats it now is because of how the AST is structured, which is based on operator precedence. If we force it to be different, we'll probably run into other weird formatting when the ternary operator is involved in a longer expression. We should create a test file with a bunch of different types of expressions involving a ternary. I have no problems changing this though, hopefully it's not too hard. I don't think it's possible to automatically add empty lines in the right place most of the time. I think this should be under the developer control to decide where to put them. Yeah, I think you're right. FWIW refmt does not do this, and I was trying to be as opinionated as it. However, I think this would be another thing that would help adoption. I think we could at least collapse multiple empty lines into a single empty line. Otherwise, keep single and double newlines in the output, which allows the programmer to group code. Invalid code generated That's not invalid! :) I'm kidding, I know what you mean. It moved the comments up. Comments are really hard, and that behavior is from recast, because it doesn't know if the comment is attached to the Remove trailing whitespace Implement call chains — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[jlongster]

Should have more time tomorrow to fix most of these; already finished two. Does the function args change look good?

fa16988

[jlongster]

Specializing last argument expansion is finished in 588c8ce (and a follow-up commit, there are a few errors in those snapshot changes).

[jlongster]

No more spaces inside objects/arrays in 8326963. You can configure this by passing the bracketSpacing option (jscodefmt --bracket-spacing will turn it on).

[jlongster]

Uses , for separating flow type fields in b550f1d

[jlongster]

Single quotes can be enabled in 599b431 with the --single-quotes option or the quote: "single" API option (default to double for now).

[jlongster]

@vjeux A few questions about trailing commas:

1. Should they only be printed if the expression is broken across newlines? So it would print the following:

// No comma
{ one: 1, two: 2 }

// Has comma
{
  one: 1,
  two: 2,
}

2. Which things should print trailing commas out of the following?

• ObjectExpressions
• ArrayExpressions
• Function args (trailing comma is illegal, right?)
• Flow tuple/object types

[jlongster]

In fc186c8 you can add trailing commas to objects split across lines with the --trailing-comma option. If there are other expressions that should have a trailing comma, let me know.

[vjeux]

@jlongster for commas:

That's correct, only print trailing comma when they are split in groups.

For function args, we have a tc39 proposal at stage 4 ("Finished: Indicate that the addition is ready for inclusion in the formal ECMAScript standard") https://github.com/tc39/proposal-trailing-function-commas and a babel transform that we've been using for years https://babeljs.io/docs/plugins/syntax-trailing-function-commas/

We want everything that is separated by commas to have trailing commas. All the ones you mentioned are √. @pieterv probably has an exhaustive list.

[jlongster]

We want everything that is separated by commas to have trailing commas. All the ones you mentioned are √. @pieterv probably has an exhaustive list.

Hm... I'm not sure I want to print trailing commas in a place that require a babel transform. I suppose I could do that, since by default it doesn't print trailing commas, and if you opt in you get it everywhere. If users complain maybe we'll have to provide a slightly more fine-grain option.

[jlongster]

else if's should be combined in fcd5436

[jlongster]

@vjeux Alright, in 0d4d21c it prints trailing commas everywhere. If I missed any place, let me know.

[jlongster]

Ternary operators will break across lines first in 7b0ec6d. This actually was easy; I just hadn't implemented any breaking behavior for ternary ops.

[jlongster]

Better rules for breaking in union/intersection types landed in 5c39dc0

[jlongster]

Added call chains in #4

[jlongster]

Original blank lines are preserverd in bcd44b4. @vjeux I'd be interested in how the nuclide codebase looks now after that; does it work properly?

[jlongster]

I improved printing of comments in 994432c. There is still probably more work to do there.

Unfortunately I could not solve this case:

this._configCache = LRU({
  max: 2000, // Want this to exceed the maximum...
  maxAge: 1000 * 30, // 30 seconds
});

See the issue on recast that I linked to before. It's very difficult because although it's attached as a trailing comment on the right object property AST node, that node is printed before the comma. Therefore a naive printing will do this:

this._configCache = LRU({
  max: 2000 // Want this to exceed the maximum...,
  maxAge: 1000 * 30 // 30 seconds,
});

Note how the comma is printed after the comment. Fixing this would involve some really complicated logic for how to interlave comments.

But it's even harder for us than recast. Since we are a flexible printer, we will collapse nodes when they fix. That means that inline comments will be collapsed as well, and even if we fixed the comma, we would output this:

this._configCache = LRU({ max: 2000, // Want this to ... maxAge: 1000 * 30 // 30 seconds })

Basically users cannot reliably have inline comments because even if we outputted them correctly, they might be collapsed. We could also try and force parents to break if they have inline comments, but all of this will be really hard.

For now, we do what recast does and simply convert those trailing comments to a leading comment which is much easier to print. We can look into improving this later, although once you apply our printer, it won't convert leading comments back to trailing ones...

[jlongster]

I think I have completed this issue as much as I can right now. Feel free to file more issues specific to comments so that we can track them. I'm sure there are other things we can do, but I'd like to go ahead and announce this to get more feedback.