feat(rome_js_parser): Support const modifier in type parameters #4275
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
723cdd2
to
554445f
Compare
}, | ||
constraint: missing (optional), | ||
default: missing (optional), | ||
TsBogusType { |
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.
These are regression, but I seem that these are not so problem.
It is better to fix, but I couldn't. Should I fix this point?
This PR is ready for review. I'm sorry for the large diff because of the CST change 🙇 |
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.
This is an outstanding contribution! Is this the last item to support TS 5.0?
Except ecma decorators, I think this PR is the last item. |
@denbezrukov would you mind giving it a second look when you can? From my point of view we can merge it soon |
|
||
impl FormatNodeRule<TsConstModifier> for FormatTsConstModifier { | ||
fn fmt_fields(&self, node: &TsConstModifier, f: &mut JsFormatter) -> FormatResult<()> { | ||
format_verbatim_node(node.syntax()).fmt(f) |
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 guess for new modifiers we can have the same format logic which we have for other modifiers.
e.g. readonly_modifier.rs
impl FormatNodeRule<TsReadonlyModifier> for FormatTsReadonlyModifier {
fn fmt_fields(&self, node: &TsReadonlyModifier, f: &mut JsFormatter) -> FormatResult<()> {
let TsReadonlyModifierFields { modifier_token } = node.as_fields();
write![f, [modifier_token.format()]]
}
}
Note:
Does prettier have any format examples of these modifiers in updated snapshots? 🤔
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 could focus on formatting in a different PR, in case there's some special formatting to take in consideration.
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.
Does prettier have any format examples of these modifiers in updated snapshots?
Prettier has not yet completed const modifier support. ref: prettier/prettier#14240
} = node.as_fields(); | ||
|
||
if let Some(in_modifier_token) = in_modifier_token { | ||
write!(f, [in_modifier_token.format(), space()])?; | ||
if modifiers.len() > 0 { |
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.
Nit: is_empty
if !is_nth_at_identifier(p, n + 1) { | ||
if is_nth_at_type_parameter_modifier(p, n + 1) && !JsSyntaxFeature::Jsx.is_supported(p) | ||
{ | ||
// <const T>... |
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 help me please to understand what a case we cover here?
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.
These lines are needed to parse the below.
<const T>() => {};
This code couldn't be parsed if not IsParenthesizedArrowFunctionExpression::True
.
Without this modification, go into !is_nth_at_identifier(p, n + 1)
condition and the function return IsParenthesizedArrowFunctionExpression::False
@@ -47,6 +51,11 @@ bitflags! { | |||
/// | |||
/// By default, 'in' and 'out' modifiers are not allowed. | |||
const ALLOW_IN_OUT_MODIFIER = 1 << 1; |
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.
It's fine if we have in
and out
inside one flag.
What do you think of the idea if it was different flags? @ematipico @nissy-dev
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'm not very familiar with the Typescript syntax, but if in
and out
can appear together in the same type, then it makes sense to have two different flags.
Instead, if they exclude each other (you can only in
or out
, not both), then it makes sense to have only one flag.
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.
Optional Variance Annotation (in/out modifier) can only be used with class, interface and type alias. In this case, the in/out modifier has the rule that out must not precede int in the order of modifiers.
class A <in T>{} // OK
class A <out T>{} // OK
class A <in out T>{} // OK
class A <out in T>{} // NG, not valid modifier order
On the other hand, Const Type Parameter (const modifier) can only be used with function, method, and class.
For my part, I feel there is no need to separate the two since this flag implies whether Optional Variance Annotation is enabled or not. It might be better to rename the flag to allow_optional_variance_annotation
or something like that. (allow_const_modifier
might be also renamed to allow_const_type_parameter
).
refs
Summary
Fix #4227
Test Plan
I added the parser test referred by babel/babel#15384
Documentation