This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(rome_js_formatter): add format element label #2783
feat(rome_js_formatter): add format element label #2783
Changes from 7 commits
ecf2789
e2ac4e6
7d06443
0dced8e
3de8d8f
4faf2c8
99874e5
670a4e7
43122bb
c887ee4
2dd6486
3f69092
e600e49
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the correct usage. This example shows that the consumer is forced to extract manually the
FormatElement
which is a low-level API for our formatter.I wonder instead if we should provide an API, something like
f.label_assert_of
/label_id.assert_of
(it asserts if the type of the label), which returns a boolean for us.The suggestion is vague because I still need to understand how we do want to use the label and it can be used inside a real world example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed this prettier API then I looked into
isPoorlyBreakableMemberOrCallChain
for variable assignment.They use label to understand that
CallExpression
has been printed as amember-chain
.I've just realized that it seems they traverse tree to extract
label
. Because they callisPoorlyBreakableMemberOrCallChain
with properties array.But current this PR API allows look only last
FormatElement
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to come up with a different solution here, I suppose. I am not sure if
printCallExpression
actually prints in their main buffer or not (it seems not, which works for their logic). Regardless, they are able to extract the IR for they right-hand side of the assignment like expression and then decide the layout.Our formatter works left-to-right, which means that once we write the right-hand side, it's there, unless we write it in a temporary buffer (which can be expensive, so we should avoid it).
I would suggest another solution for the assignment case. They create the label only when they actually create a member chain. They don't create the member chain in a specific case: https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/member-chain.js#L342-L347
That condition is this one: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/src/utils/member_chain/groups.rs#L42-L48
If we are able to use that logic inside the assignment like formatting, we might be able to not use the label.
Otherwise, the only solution that I can see is the write the right-hand side inside a temporary buffer, than inspect that buffer and check the label. But as said before, this has a big impact on memory usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense👍
I guess that we can try it (:
What do you think about another case?
Now we have
should_not_indent_if_parent_indents
inbinary_like_expression
module. This function is aware about place where it will be printed. It usesshould_break_after_operator
fromassignment_like
module because whenbinary_like_expression
is right part and layout isBreakAfterOperator
assignment_like
adds indent and to avoid double indent we have to check the same logic inbinary_like_expression
. May be we can invert this dependency and uselabel
forbinary_like_expression
that it already has indent and check this label inassignment_like
module.tools/crates/rome_js_formatter/src/utils/binary_like_expression.rs
Lines 263 to 284 in 58c297f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if that would work. Mainly because at the end of the format phase, where that function is used, the logic might add parenthesis to the binary expression, and the indentation has to stay inside the parenthesis. That's why checking the AST is better in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Thank you!
So to emulate prettier
label
functionality we can add new extensioninspect_label
,InspectLabelBuffer
and method forFormatElement
has_label
which traverses IR tree and search expected label?EDIT:
It seems that second case also doesn't write in the main buffer.
tools/crates/rome_formatter/src/format_extensions.rs
Lines 217 to 227 in c518066
tools/crates/rome_formatter/src/formatter.rs
Lines 166 to 173 in c518066
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what Prettier does? Does it traverse ALL the IR in order to find a label? I thought it just checks the first element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you're right.
I double checked and prettier checks the first element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please help me?🙏🏽 I can't find this case in Rome code:
Because it seems this conditional:
is prettier:
UPDATE:
Do I understand correctly that prettier uses only one array for all groups and Rome uses two structs (
HeadGroup
andGroups
)? I was wondering aboutcutoff
value. It can be2
and3
depends onshould_merge
.It seems that it always should be
1
. Because Rome uses two structs and thenshould_merge
is true it mutatesGroups
vec.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
shouldMerge
value is used to essentially decide the layout of the formatting, and this affects the head of the group. So I went for a different approach because prettier's one was not working for us.Yes, in theory
cutoff
should not be needed anymore, now that we actually mutate the groups vectorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unstable api to make this function
const
.const fn
type_id
rust-lang/rust#77125There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks: