feat(rome_js_parser): Support const modifier in type parameters #4275
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
nissy-dev
changed the title
nissy-dev
changed the title
github-actions
bot
added
A-Formatter
nissy-dev
force-pushed
the
feat-const-modifier
branch
from
723cdd2
to
554445f
Compare
| }, | ||
| constraint: missing (optional), | ||
| default: missing (optional), | ||
| TsBogusType { |
There was a problem hiding this comment.
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 🙇 |
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.
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.
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.
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 { |
| 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.
Could you help me please to understand what a case we cover here?
There was a problem hiding this comment.
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.
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.
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.
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 orderOn 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