Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Destructuring arguments #2439

Closed
Tracked by #2403
MichaReiser opened this issue Apr 14, 2022 · 6 comments · Fixed by #2711
Closed
Tracked by #2403

Destructuring arguments #2439

MichaReiser opened this issue Apr 14, 2022 · 6 comments · Fixed by #2711
Assignees
Labels
A-Formatter Area: formatter I-Normal Implementation: normal understanding of the tool and awareness

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Apr 14, 2022

Rome's formatter always tries to fit all restructuring members on a new line whereas Prettier has a heuristic to special print restructuring patterns when it's the only function argument Playground

Input

// Only argument, whole definition doesn't fit. Print [ or { on same line, members on a new line each
function excludeFirstFiveResults3([
  firstResult, secondResult, thirdResult, fourthResult, fifthResult, ...rest] = [1, 2, 3, 4, 5]) {
}


function StatelessFunctionalComponent({
  isActive,
  onFiltersUpdated,
  onSelect,
}) {
  return <div />
}

// If the function has more than one argument, try to fit destructuring on a single line 
// If not, print each member on their own line. 
function excludeFirstFiveResults3([
  firstResult, secondResult, thirdResult, fourthResult, fifthResult, ...rest] = [1, 2, 3, 4, 5], test, third) {
}

function StatelessFunctionalComponent2({
  isActive,
  onCancel,
  searchFilters,
  title,
  items,
}, more) {
  return <div />
}

function StatelessFunctionalComponent2(first, {
  isActive,
  onCancel,
  searchFilters,
  title,
  items,
}) {
  return <div />
}

Prettier

// Only argument, whole definition doesn't fit. Print [ or { on same line, members on a new line each
function excludeFirstFiveResults3(
  [
    firstResult,
    secondResult,
    thirdResult,
    fourthResult,
    fifthResult,
    ...rest
  ] = [1, 2, 3, 4, 5]
) {}

function StatelessFunctionalComponent({
  isActive,
  onFiltersUpdated,
  onSelect,
}) {
  return <div />;
}

// If the function has more than one argument, try to fit destructuring on a single line
// If not, print each member on their own line.
function excludeFirstFiveResults3(
  [
    firstResult,
    secondResult,
    thirdResult,
    fourthResult,
    fifthResult,
    ...rest
  ] = [1, 2, 3, 4, 5],
  test,
  third
) {}

function StatelessFunctionalComponent2(
  { isActive, onCancel, searchFilters, title, items },
  more
) {
  return <div />;
}

function StatelessFunctionalComponent2(
  first,
  { isActive, onCancel, searchFilters, title, items }
) {
  return <div />;
}

Rome

// Only argument, whole definition doesn't fit. Print [ or { on same line, members on a new line each
function excludeFirstFiveResults3(
  [firstResult, secondResult, thirdResult, fourthResult, fifthResult, ...rest] = [
    1, 2, 3, 4, 5,
  ],
) {}

function StatelessFunctionalComponent({ isActive, onFiltersUpdated, onSelect }) {
  return <div />;
}

// If the function has more than one argument, try to fit destructuring on a single line
// If not, print each member on their own line.
function excludeFirstFiveResults3(
  [firstResult, secondResult, thirdResult, fourthResult, fifthResult, ...rest] = [
    1, 2, 3, 4, 5,
  ],
  test,
  third,
) {}

function StatelessFunctionalComponent2(
  { isActive, onCancel, searchFilters, title, items },
  more,
) {
  return <div />;
}

function StatelessFunctionalComponent2(
  first,
  { isActive, onCancel, searchFilters, title, items },
) {
  return <div />;
}

Expected

Rome's formatting to match Prettier's

@ematipico ematipico added the A-Formatter Area: formatter label Apr 14, 2022
@ematipico ematipico self-assigned this Apr 14, 2022
@ematipico
Copy link
Contributor

Actually, this is the expected behaviour and the issue not the destructuring per se. We are hitting the known limitation that Leo already talked about.

Try this instead and you will see that we actually print correctly: Playground

@MichaReiser
Copy link
Contributor Author

@ematipico can you elaborate? Prettier has very specific formatting if the only parameter is an object or array pattern that Rome doesn't support.

@ematipico
Copy link
Contributor

ematipico commented Apr 14, 2022

Here's a link to the message. In this case the closing ] it's not considered inside the grouping when considering the max width.

Maybe you're talking about a specific case that I can't see. If you add sixthResult to the destructuring, you will see that we format like prettier.

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Apr 14, 2022

Here's a link to the message. In this case the closing ] it's not considered inside the grouping when considering the max width.

Maybe you're talking about a specific case that I can't see. If you add sixthResult to the destructuring, you will see that we format like prettier.

This

Prettier has a heuristic to special print restructuring patterns when it's the only function argument

The difference is that Prettier breaks the argument if the whole function header doesn't fit, and not just the parameters. This isn't related to the case where Rome prints a few characters over the line width.

@ematipico
Copy link
Contributor

Here's a link to the message. In this case the closing ] it's not considered inside the grouping when considering the max width.
Maybe you're talking about a specific case that I can't see. If you add sixthResult to the destructuring, you will see that we format like prettier.

This

Prettier has a heuristic to special print restructuring patterns when it's the only function argument

The difference is that Prettier breaks the argument if the whole function header doesn't fit, and not just the parameters. This isn't related to the case where Rome prints a few characters over the line width.

Can you add the case into the description? Because this actually highlights the issue, but the case the example I shared actually hits the limitation we just spoke and I think it is not related to this issue in particular. They are two different cases, the IR is different for Prettier too.

@MichaReiser
Copy link
Contributor Author

Here's a link to the message. In this case the closing ] it's not considered inside the grouping when considering the max width.
Maybe you're talking about a specific case that I can't see. If you add sixthResult to the destructuring, you will see that we format like prettier.

This

Prettier has a heuristic to special print restructuring patterns when it's the only function argument

The difference is that Prettier breaks the argument if the whole function header doesn't fit, and not just the parameters. This isn't related to the case where Rome prints a few characters over the line width.

Can you add the case into the description? Because this actually highlights the issue, but the case the example I shared actually hits the limitation we just spoke and I think it is not related to this issue in particular. They are two different cases, the IR is different for Prettier too.

The case is mentioned in the description and this issue aren't about specific technical limitations but how our formatter formats things differently. That means, who ever works on this needs to take a closer look on how patterns are formatted in function parameters. If we have to create issue for every distinct case then we end up with 100 of issues and it would take me weeks.

@ematipico ematipico added the I-Normal Implementation: normal understanding of the tool and awareness label May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter I-Normal Implementation: normal understanding of the tool and awareness
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants