Skip to content

Avoid extra offset in arrow function body when using long types (alternative solution)#11515

Merged
sosukesuzuki merged 9 commits intoprettier:mainfrom
thorn0:alt-10848
Nov 17, 2021
Merged

Avoid extra offset in arrow function body when using long types (alternative solution)#11515
sosukesuzuki merged 9 commits intoprettier:mainfrom
thorn0:alt-10848

Conversation

@thorn0
Copy link
Copy Markdown
Member

@thorn0 thorn0 commented Sep 13, 2021 •
edited
Loading

Description

This is an alternative to #10956. It uses the following formatting:

const MyComponent2: React.VoidFunctionComponent<
  MyComponent2Props
> = ({ x, y }) => {
  const a = useA();
  return (
    <div>
      x = {x}; y = {y}; a = {a}
    </div>
  );
};

@kachkaev How does this look to you?

Regression testing (prettier/prettier-regression-testing#73 (comment)) has shown that "prefer to keep the arrow signature on one line if possible" is a bad idea in this case.

Testing results for the current version of this PR: prettier/prettier-regression-testing#73 (comment)

Closes #10848

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✨

@github-actions github-actions Bot mentioned this pull request Sep 13, 2021
Closed
@thorn0 thorn0 marked this pull request as draft September 13, 2021 01:59
@thorn0
Don't try to keep arrow signatures on one line
aa9842d 
Regression testing showed that this was a bad idea in this case
@thorn0
Add changelog
8751c8a
@thorn0 thorn0 marked this pull request as ready for review September 13, 2021 11:08
@kachkaev
Copy link
Copy Markdown
Member

kachkaev commented Sep 13, 2021

👋 @thorn0! The diff in integration tests makes absolute sense to me, so I’m more than happy to proceed with your solution 😍

@kachkaev kachkaev mentioned this pull request Sep 13, 2021
Closed
3 tasks
sosukesuzuki
sosukesuzuki reviewed Sep 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! But diffs are large, so I think we should release this as minor.

Comment thread src/language-js/print/type-parameters.js Outdated
@kachkaev
Copy link
Copy Markdown
Member

kachkaev commented Sep 14, 2021 •
edited
Loading

@sosukesuzuki this PR fixes a regression in Prettier 2.3.0 – the new behaviour closely matches 2.2.1. Given this and the fact that 2.4.0 is quite new, would it be reasonable to include the PR into 2.4.1?

If this was a completely new behaviour, I'd agree with you, but a former change between 2.2 and 2.3 tweaks the weights a bit I guess 🙂

alexander-akait
alexander-akait approved these changes Sep 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

@sosukesuzuki
Copy link
Copy Markdown
Contributor

sosukesuzuki commented Sep 14, 2021

Hmmm... I still think this should be released in 2.5, as it is not a fix for the regressions that occurred in 2.4, and it would create a large diff. Many users will not like a big diff in a patched version. (But I'm not confident that this is the best way).

@sosukesuzuki sosukesuzuki added this to the 2.5 milestone Sep 22, 2021
@sosukesuzuki sosukesuzuki merged commit 11e43e2 into prettier:main Nov 17, 2021
@kachkaev kachkaev mentioned this pull request Dec 4, 2021
Open
@jsnajdr jsnajdr mentioned this pull request Apr 22, 2022
Open
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 18, 2022
@thorn0 thorn0 deleted the alt-10848 branch December 28, 2022 13:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

@sosukesuzuki sosukesuzuki sosukesuzuki approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.5

Development

Successfully merging this pull request may close these issues.

The offset for React component body depends on component name length in v2.3

4 participants

@thorn0 @kachkaev @sosukesuzuki @alexander-akait