Prevent wrapping object properties with short keys by thorn0 · Pull Request #10335 · prettier/prettier

thorn0 · 2021-02-17T11:19:16Z

Description

Fixes #3711
It's not obvious from the changes in the snapshots, but this is able to lead to huge diffs. Still worth doing IMHO.

Checklist

I’ve added tests to confirm my change works.
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

alexander-akait · 2021-02-17T11:30:43Z

src/language-js/print/assignment.js

+    node.key &&
+    !node.computed &&
+    node.key.type === "Identifier" &&
+    node.key.name.length <= options.tabWidth + 2


Why 2? 😄

It was 3 initially. :D Suggestions?

There has to be a sufficient visual overlap for the line break to look natural. I'm having trouble with determining how long exactly it needs to be.

Honestly, I also have no ideas, magic numbers scare me 😄 , maybe we just need to make cases with 2/3/4 values and look where it will be better

Added that to snapshots. Have a look.

I guess the value 2 was based on this comment #3711 (comment)

A sample diff: prettier/prettier-regression-testing#32 (comment)

alexander-akait · 2021-02-17T11:54:35Z

tests/js/object-prop-break-in/__snapshots__/jsfmt.spec.js.snap

+  a: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  ab: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abc: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcd: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",


'abcd': "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",

Should be same?

I don't think so.

The overall length of the part before : is measured.

abcd → 4 'abcd' → 6 [x] → 3

fisker · 2021-02-17T12:18:29Z

src/language-js/print/assignment.js

+  }
+  // TODO: a more lightweight version of cleanDoc is needed that would stop once it's
+  // detected that the doc can't be normalized into a string.
+  keyDoc = cleanDoc(keyDoc);


Use printDocToString?

Doesn't sound like an optimization

Doesn't, but don't relay on how the doc is build.

printDocToString always returns a string, but I need to check that the doc consists only of strings. No line breaks, etc. How is that worse than, for example, willBreak? Doc structure checks are everywhere in Prettier's code.

For example label.

label is for complex things like member chains. We definitely don't want to consider such keys "simple".

src/language-js/print/assignment.js

Co-authored-by: Sosuke Suzuki <[email protected]>

sosukesuzuki

String length causes a strange bug as below, but I think we can ignore it in the real world 😄

Prettier pr-10335
Playground link

--parser babel

Input:

foo = {
  // length 5
  foooo: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
  // length 4
  fooo: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
  // length 4? no, length 5!
  "𩸽o": "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
};

Output:

foo = {
  // length 5
  foooo:
    "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
  // length 4
  fooo: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
  // length 4? no, length 5!
  "𩸽o":
    "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
};

thorn0 · 2021-02-18T12:10:28Z

@sosukesuzuki We have getStringWidth for that. Good catch.

thorn0 · 2021-02-18T18:23:08Z

getStringWidth returns 2 for that character too because it's full-width. So the output was actually correct. Now it's correct also for other full-width characters. On the other hand, the preserved quotes actually look like a bug. Shouldn't they have been removed?

upd: Turns out Prettier conservatively uses rules from ES5.1 to decide what can be unquoted.

lipis

:100

thorn0 marked this pull request as draft February 17, 2021 11:26

thorn0 force-pushed the assignments3 branch from 6f2e6ac to 989c754 Compare February 17, 2021 11:29

alexander-akait reviewed Feb 17, 2021

View reviewed changes

thorn0 force-pushed the assignments3 branch from 989c754 to 5f41729 Compare February 17, 2021 11:48

alexander-akait reviewed Feb 17, 2021

View reviewed changes

fisker reviewed Feb 17, 2021

View reviewed changes

thorn0 force-pushed the assignments3 branch from 5f41729 to f670f2c Compare February 17, 2021 13:15

github-actions bot mentioned this pull request Feb 17, 2021

pr 10335 -- Prevent wrapping object properties with short keys prettier/prettier-regression-testing#32

Closed

thorn0 force-pushed the assignments3 branch from f670f2c to c773874 Compare February 17, 2021 14:46

thorn0 marked this pull request as ready for review February 17, 2021 14:46

thorn0 force-pushed the assignments3 branch 2 times, most recently from e76f87a to 4098493 Compare February 17, 2021 16:05

Prevent wrapping object properties with short keys

1e5534d

thorn0 force-pushed the assignments3 branch from 4098493 to 1e5534d Compare February 18, 2021 02:29

sosukesuzuki reviewed Feb 18, 2021

View reviewed changes

src/language-js/print/assignment.js Outdated Show resolved Hide resolved

Update src/language-js/print/assignment.js

21013c4

Co-authored-by: Sosuke Suzuki <[email protected]>

sosukesuzuki approved these changes Feb 18, 2021

View reviewed changes

Use getStringWidth instead of .length

5aabad3

lipis approved these changes Mar 1, 2021

View reviewed changes

thorn0 merged commit 5f8fee4 into prettier:main Mar 2, 2021

thorn0 deleted the assignments3 branch March 2, 2021 23:43

fisker added a commit to fisker/prettier that referenced this pull request Apr 6, 2021

Remove prettier#10335

a46e75e

fisker mentioned this pull request Apr 6, 2021

Main branch without 10335 prettier/prettier-regression-testing#54

Open

sosukesuzuki mentioned this pull request Aug 11, 2021

Inconsistent line breaks for object literal between JS and JSON #11333

Open

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2022

Uh oh!

Conversation

thorn0 commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

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!

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!

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!

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!

Uh oh!

sosukesuzuki left a comment

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Feb 18, 2021

Uh oh!

thorn0 commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lipis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thorn0 commented Feb 17, 2021 •

edited

Loading

thorn0 commented Feb 18, 2021 •

edited

Loading