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

Commit

Permalink
fix(rome_js_formatter): break computed expressions like prettier (#2517)
Browse files Browse the repository at this point in the history
  • Loading branch information
ematipico authored May 2, 2022
1 parent 4ace7e4 commit 58ce2f2
Show file tree
Hide file tree
Showing 10 changed files with 160 additions and 69 deletions.
Original file line number Diff line number Diff line change
@@ -1,29 +1,70 @@
use crate::format_traits::FormatOptional;
use rome_formatter::FormatResult;
use rome_formatter::{
concat_elements, group_elements, soft_block_indent, soft_line_break, FormatResult,
};

use crate::{format_elements, Format, FormatElement, FormatNode, Formatter};

use rome_js_syntax::JsComputedMemberExpression;
use rome_js_syntax::JsComputedMemberExpressionFields;
use rome_rowan::AstNode;

impl FormatNode for JsComputedMemberExpression {
fn format_fields(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
let mut current = self.clone();

// Find the left most computed expression
while let Some(computed_expression) =
JsComputedMemberExpression::cast(current.object()?.syntax().clone())
{
current = computed_expression;
}

// Format the left most computed expression
let JsComputedMemberExpressionFields {
object,
optional_chain_token,
l_brack_token,
member,
r_brack_token,
} = self.as_fields();
} = current.as_fields();

let optional_chain_token = optional_chain_token.format_or_empty(formatter)?;

Ok(format_elements![
let mut formatted = vec![format_elements![
object.format(formatter)?,
optional_chain_token,
l_brack_token.format(formatter)?,
member.format(formatter)?,
r_brack_token.format(formatter)?,
])
group_elements(format_elements![
optional_chain_token.format_or_empty(formatter)?,
l_brack_token.format(formatter)?,
soft_line_break(),
soft_block_indent(member.format(formatter)?),
r_brack_token.format(formatter)?,
]),
]];

// Traverse upwards again and concatenate the computed expression until we find the first non-computed expression
while let Some(parent) = current.syntax().parent() {
if let Some(parent_computed) = JsComputedMemberExpression::cast(parent) {
let JsComputedMemberExpressionFields {
object: _,
optional_chain_token,
l_brack_token,
member,
r_brack_token,
} = parent_computed.as_fields();

formatted.push(group_elements(format_elements![
optional_chain_token.format_or_empty(formatter)?,
l_brack_token.format(formatter)?,
soft_line_break(),
soft_block_indent(member.format(formatter)?),
r_brack_token.format(formatter)?,
]));

current = parent_computed;
} else {
break;
}
}

Ok(concat_elements(formatted))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,9 @@ foo.bar["bar"]["lorem_ispsum"].foo["lorem-ipsum"] = true;

a[ b ]
c?.[ d ]

x["very"]["long"]["chain"]["of"]["computed"]["members"]["that"]["goes"]["on"]["for"]["ever"]["I"]["mean"]["it"]["for"]["ever"]["and"]["even"]["longer"]["than"]["that"]

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever["and"].even.longer.than.that.and["rofefefeeffeme"].doesnt.supportit

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[and()].even.longer.than.that.and["rofefefeefefme"].doesnt.supportit
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 242
expression: computed_member.js
---
# Input
Expand All @@ -12,6 +13,12 @@ foo.bar["bar"]["lorem_ispsum"].foo["lorem-ipsum"] = true;
a[ b ]
c?.[ d ]

x["very"]["long"]["chain"]["of"]["computed"]["members"]["that"]["goes"]["on"]["for"]["ever"]["I"]["mean"]["it"]["for"]["ever"]["and"]["even"]["longer"]["than"]["that"]

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever["and"].even.longer.than.that.and["rofefefeeffeme"].doesnt.supportit

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[and()].even.longer.than.that.and["rofefefeefefme"].doesnt.supportit

=============================
# Outputs
## Output 1
Expand All @@ -29,3 +36,17 @@ foo.bar["bar"]["lorem_ispsum"].foo["lorem-ipsum"] = true;
a[b];
c?.[d];

x["very"]["long"]["chain"]["of"]["computed"]["members"]["that"]["goes"]["on"][
"for"
]["ever"]["I"]["mean"]["it"]["for"]["ever"]["and"]["even"]["longer"]["than"][
"that"
];

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[
"and"
].even.longer.than.that.and["rofefefeeffeme"].doesnt.supportit;

x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[
and()
].even.longer.than.that.and["rofefefeefefme"].doesnt.supportit;

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 175
expression: issue-7091.js
---
# Input
Expand All @@ -12,12 +13,10 @@ const {

# Output
```js
const { imStore, showChat, customerServiceAccount } = store[config.reduxStoreName];
const { imStore, showChat, customerServiceAccount } = store[
config.reduxStoreName
];
```

# Lines exceeding max width of 80 characters
```
1: const { imStore, showChat, customerServiceAccount } = store[config.reduxStoreName];
```

Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 125
assertion_line: 175
expression: comment.js

---
# Input
```js
Expand Down Expand Up @@ -97,14 +96,18 @@ var a = x(
bifornCringerMoshedPerplexSawder,
);
foo[a +
a + // comment
foo[
a +
bar[b +
b +
b + // comment
b +
b]];
a + // comment
a +
bar[
b +
b +
b + // comment
b +
b
]
];
!(
a +
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 175
expression: expand.js
---
# Input
Expand Down Expand Up @@ -44,7 +45,9 @@ window.FooClient.something.setVars({

# Output
```js
const veryVeryVeryVeryVeryVeryVeryLong = doc.expandedStates[doc.expandedStates.length - 1];
const veryVeryVeryVeryVeryVeryVeryLong = doc.expandedStates[
doc.expandedStates.length - 1
];
const small = doc.expandedStates[doc.expandedStates.length - 1];
const promises = [
Expand Down Expand Up @@ -94,8 +97,4 @@ window.FooClient.something.setVars({
```

# Lines exceeding max width of 80 characters
```
1: const veryVeryVeryVeryVeryVeryVeryLong = doc.expandedStates[doc.expandedStates.length - 1];
```

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 175
expression: break-last-member.js
---
# Input
Expand Down Expand Up @@ -30,9 +31,13 @@ const {
# Output
```js
SomeVeryLongUpperCaseConstant.someVeryLongCallExpression().some_very_long_member_expression;
weNeedToReachTheEightyCharacterLimitXXXXXXXXXXXXXXXXX.someNode.childrenInAnArray[0];
weNeedToReachTheEightyCharacterLimitXXXXXXXXXXXXXXXXX.someNode.childrenInAnArray[
0
];
superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered;
superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered[0];
superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered[
0
];
expect(
findDOMNode(component.instance()).getElementsByClassName(styles.inner)[0].style.paddingRight,
Expand All @@ -54,11 +59,11 @@ const {
# Lines exceeding max width of 80 characters
```
1: SomeVeryLongUpperCaseConstant.someVeryLongCallExpression().some_very_long_member_expression;
2: weNeedToReachTheEightyCharacterLimitXXXXXXXXXXXXXXXXX.someNode.childrenInAnArray[0];
3: superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered;
4: superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered[0];
7: findDOMNode(component.instance()).getElementsByClassName(styles.inner)[0].style.paddingRight,
10: const { course, conflicts = [], index, scheduleId, studentId, something } = a.this.props;
12: const { course2, conflicts2 = [], index2, scheduleId2, studentId2, something2 } = this.props;
2: weNeedToReachTheEightyCharacterLimitXXXXXXXXXXXXXXXXX.someNode.childrenInAnArray[
5: superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered;
6: superSupersuperSupersuperSupersuperSupersuperSuperLong.exampleOfOrderOfGetterAndSetterReordered[
11: findDOMNode(component.instance()).getElementsByClassName(styles.inner)[0].style.paddingRight,
14: const { course, conflicts = [], index, scheduleId, studentId, something } = a.this.props;
16: const { course2, conflicts2 = [], index2, scheduleId2, studentId2, something2 } = this.props;
```

Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ const foo = `such a long template string ${foo.bar.baz} that prettier will want
const shouldWrapForNow = `such a long template string ${foo().bar.baz} that prettier will want to wrap it`;
const shouldNotWrap = `simple expressions should not break ${this} ${variable} ${a.b.c} ${this.b.c} ${a[b].c} ${a.b[c]} ${a.b["c"]} ${a?.b?.c}`;
const shouldNotWrap = `simple expressions should not break ${this} ${variable} ${a.b.c} ${this.b.c} ${a[
b
].c} ${a.b[c]} ${a.b["c"]} ${a?.b?.c}`;
console.log(
chalk.white(
Expand Down Expand Up @@ -132,11 +134,11 @@ throw new Error(
8: const description = `The value of the ${cssName} css of the ${this._name} element`;
10: const foo = `such a long template string ${foo.bar.baz} that prettier will want to wrap it`;
12: const shouldWrapForNow = `such a long template string ${foo().bar.baz} that prettier will want to wrap it`;
14: const shouldNotWrap = `simple expressions should not break ${this} ${variable} ${a.b.c} ${this.b.c} ${a[b].c} ${a.b[c]} ${a.b["c"]} ${a?.b?.c}`;
18: `Covered Lines below threshold: ${coverageSettings.lines}%. Actual: ${coverageSummary.total.lines.pct}%`,
32: return `${process.env.OPENID_URL}/something/something/something?${Object.keys(
42: `Trying update appcast for ${app.name} (${app.cask.appcast}) -> (${app.cask.appcastGenerated})`,
50: `\nApparently jetbrains changed the release artifact for ${app.name}@${app.jetbrains.version}.\n`,
64: `pretty-format: Option "theme" has a key "${key}" whose value "${value}" is undefined in ansi-styles.`,
14: const shouldNotWrap = `simple expressions should not break ${this} ${variable} ${a.b.c} ${this.b.c} ${a[
20: `Covered Lines below threshold: ${coverageSettings.lines}%. Actual: ${coverageSummary.total.lines.pct}%`,
34: return `${process.env.OPENID_URL}/something/something/something?${Object.keys(
44: `Trying update appcast for ${app.name} (${app.cask.appcast}) -> (${app.cask.appcastGenerated})`,
52: `\nApparently jetbrains changed the release artifact for ${app.name}@${app.jetbrains.version}.\n`,
66: `pretty-format: Option "theme" has a key "${key}" whose value "${value}" is undefined in ansi-styles.`,
```
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 175
expression: indent-after-paren.js
---
# Input
Expand Down Expand Up @@ -519,7 +520,9 @@ foo =
)()()();
foo =
foo.bar.baz[coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz];
foo.bar.baz[
coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz
];
const decorated = (arg, ignoreRequestError) => {
return (
Expand Down Expand Up @@ -559,22 +562,30 @@ fn?.(
);
bifornCringerMoshedPerplexSawder =
fn[(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop];
fn[
(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop
];
fn[(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop];
fn[
(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop
];
bifornCringerMoshedPerplexSawder =
fn?.[(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop];
fn?.[
(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop
];
fn?.[(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop];
fn?.[
(
glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
).prop
];
```
Expand Down Expand Up @@ -618,14 +629,14 @@ fn?.[(
203: glimseGlyphsHazardNoopsTieTie === 0 && kochabCooieGameOnOboleUnweave === Math.PI ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
210: coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz
215: coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz
219: foo.bar.baz[coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz];
235: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
241: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
248: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
254: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
260: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
264: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
220: coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz
237: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
243: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
250: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
256: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
263: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
269: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
273: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
276: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
282: glimseGlyphsHazardNoopsTieTie === 0 ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol
```
Loading

0 comments on commit 58ce2f2

Please sign in to comment.