-
Notifications
You must be signed in to change notification settings - Fork 662
feat(rome_js_parser): support property init in ambient context #4219
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
@nissy-dev If you have some time to review my PR this could be great :) |
Note: I missed the following draft : #3000 and discussion. I will take a look when I got some time. |
I've tried to dig around in previous discussions. We can also add an error test case:
|
I have a preference for adding an optional initializer clause to
Unfortunately it is not possible to restrict ambient property initializer to literal values. Indeed, an
+1 |
I read some discussion and I think it is better to avoid the new type node and your implementation decision is almost good for me! The following comments (#3003 (comment)) is the same as my thought.
I think @denbezrukov is a more appropriate reviewer for the code details and I will leave it to them 🙏 |
Actually I also don't know which implementation is better either. Adding a new optional field is easier because we don't need to support a new node. If we already have such nodes I think that it's okay =) cc @ematipico What do you think? |
This is a good mark. Is there other case of two node living in one in the Rome CST? In my view fewer node at the price of type-safety is better. If there is other case of two node living in one in the Rome codebase, I could keep the current decision of adding an optional field to an existing node. |
Both approaches come with different downsides and advantages. Having a new node allows us to design better grammar from the beginning, making the parsing phase way easier; on the other hand, it creates a level of indirection. That's why Micha suggested creating a new node: the rules around this grammar are so subtle that it's better to have a new node with its rules. I would create a new node, but I don't feel strongly about the implementation. I'm OK with this implementation too.
What do you mean by that? |
crates/rome_js_formatter/tests/specs/ts/class/readonly_ambient_property.ts
Outdated
Show resolved
Hide resolved
...s/rome_js_parser/test_data/inline/err/ts_annotated_property_initializer_ambient_context.rast
Show resolved
Hide resolved
Are there others nodes that could be splitted into several ones in the Rome codebase? |
Not sure I understand correctly, sorry; usually we use the |
I try to understand which approach is most consistent with the previous decisions made in the Rome codebase (intentionally or implicitly). As noted by denbezrukov, two nodes live in one in the current proposal:
If we have other cases like this in the codebase, I could conclude that it is ok to introduce a new one. On the contrary, if there are no other cases like this, it seems more "Romy" to introduce a new node. |
Oh, I see what you mean now. AFAIK, we don't have an example precisely like this; the logic around the modifiers ( There is a wrong answer, and we usually decide based on the pros and cons of both approaches. TypeScript has many quirks; sometimes, we had to find the right balance between creating new nodes or having more logic inside the parser. I would create a new node for the following reasons:
|
I created another PR #4225 that makes the addition of a new node. @ematipico I let you take the final decision. |
The alternative approach was chosen. See #4225. |
Summary
Fixes #2958
Note: It is the first time I modify the parser and formatter.
Test Plan
Parser and formatter tests included.
Documentation
No change