feat(js_formatter): provide the JS formatter with the ability to format other languages #3403
Conversation
github-actions
bot
added
A-Project
CodSpeed Performance ReportMerging #3403 will not alter performanceComparing Summary
|
| biome_css_formatter = { path = "../biome_css_formatter" } | ||
| biome_css_parser = { path = "../biome_css_parser" } |
There was a problem hiding this comment.
I'm wondering if it could be a circular dependency problem at some point.
There was a problem hiding this comment.
Actually, I dont know. I'm not familiar with cargo. Does a dev dependency participate in the build phase? If there is a cycle, will cargo report it?
There was a problem hiding this comment.
Cycles aren't allowed, and rust won't compile until it's resolved.
As for dev deps, I believe that's the same because the code is compiled, even for tests.
We have options:
- move tests inside
biome_service - use the workspace
The first option is maybe the best one. I would also argue that embedded languages are an esoteric area and they should not be part of the testing of a specific language - in this example, the JavaScript language
There was a problem hiding this comment.
Cycles aren't allowed, and rust won't compile until it's resolved.
Thank you! TIL
The first option is maybe the best one
Agree
There was a problem hiding this comment.
The more I think about it, the more I am inclined to believe that it may be okay to have CSS dependencies in the dev dependencies for tests? It's simpler and prevents the JS formatter tests from being spread across the project. If we move it to another crate, how will we collect statistics about Prettier compatibility?
| @@ -44,12 +45,19 @@ pub struct JsFormatContext { | |||
| cached_function_body: Option<(AnyJsFunctionBody, FormatElement)>, | |||
|
|
|||
| source_map: Option<TransformSourceMap>, | |||
|
|
|||
| foreign_language_formatter: Rc<dyn JsForeignLanguageFormatter>, | |||
There was a problem hiding this comment.
I'm just curious why Rc<dyn> is a better choice over other options?
There was a problem hiding this comment.
I didn't think about it deeply and just followed the comments. Any better suggestions?
There was a problem hiding this comment.
Yeah, I also noticed the comments and couldn't find an explanation.
Unfortunately, I don't have any suggestions about it.
There are several options:
- Have a
<T>and have a constraint overTin the impls. Box<dyn>?
There was a problem hiding this comment.
Oh, sorry, you mean Rc<dyn>. I thought you were asking why use Rc instead of another smart pointer.
Initially, I wanted to use a closure, but I encountered a problem with deriving the Clone trait, so I chose to use Rc<dyn> instead. Rust is hard 😢
There was a problem hiding this comment.
There are several options:
Have a and have a constraint over T in the impls.
Box?
Thank you! I will think about it!
There was a problem hiding this comment.
Oh, I got it. Indeed, if we need the Clone trait, we can't use Box :(
It may be fine to use Rc.
There was a problem hiding this comment.
It seems dyn has trade-offs.
Still need to think about it, convert this PR to a draft temporarily
There was a problem hiding this comment.
Have a T and have a constraint over T in the impls.
If we attach a generic type to JsFormatContext, we also need to attach a generic type to many other types associated with JsFormatContext, for example FormatJsSyntaxToken.
The rust book mentions that dyn has runtime cost but I have no idea how much the cost it is. Wonder if the cost is acceptable while keeping the code flexible
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
The rust book mentions that dyn has runtime cost but I have no idea how much the cost it is. Wonder if the cost is acceptable while keeping the code flexible
It looks like the overhead will be totally acceptable in this case, so I’d just stick with the Rc<dyn>. After all, it’s a single pointer to the formatter that should be used, but it shouldn’t affect the performance of the formatter itself.
Generally in Rust, don’t worry too much about performance unless you’re making clones all over the place or something is becoming noticeably slow :)
|
This is great to see. If this is merged by the time I get back from vacation, I'll replicate this approach for HTML. |
Good to know! I still need some time to explore whether there is a better implementation. Any suggestions are welcome |
Summary
Part of #3334
Now you can use
f.context().get_foreign_language_formatter().format(Css, "")to format CSS codeTest Plan
CI should pass