feat(js_formatter): explore embedded language formatting #3228
Conversation
github-actions
bot
added
A-CLI
| @@ -147,15 +147,16 @@ const StyledComponent1 = styled.div` | |||
| } | |||
| `; | |||
|
|
|||
| const StyledComponent2 = styled.div` | |||
| ${anInterpolation} | |||
| // TODO: reformat issue | |||
CodSpeed Performance ReportMerging #3228 will not alter performanceComparing Summary
|
|
😻😻😻 |
|
Wow @ah-yu!! You're the man!!! 👑 Let us know if you need any help |
|
Hello @ah-yu, Great job on taking on this task! I've had some experience with embedded language formatting in Prettier, so please allow me to share a few thoughts. Personally, I'm not a fan of the Moreover, due to this lack of transparency and the fact that there are only Given this background, and motivated by the need to format XML embedded within JavaScript back then, I developed a Prettier plugin,
In simple terms, this plugin explicitly configures which parts of the code are considered embedded languages. In my plugin, I specified that only tagged templates like This plugin relies on Prettier's extensive language support plugins to successfully provide embedded code formatting for many languages. The most popular seems to be the SQL family of languages, which is also a highly requested feature by Prettier users: prettier/prettier#12139. Regarding this, I think @karlhorky can provide more insights from a developer's perspective. Regarding your work on Biome, I see that you have referenced Prettier's approach by introducing the One more thing, I noticed in your code comments for handling template strings, you mentioned replacing interpolation expressions with placeholders, then parsing, formatting, and finally replacing them back with the original expressions. This method is indeed what Prettier and In all, these are my personal insights. Although I have limited knowledge of parsers and formatters, based on my experience developing the Thanks! ❤️ |
|
Oh nice, didn't know about this PR! Cool to see more work being done in the embedded language formatting space 🙌
Ahh yeah I would also not recommend the hardcoded / Prettier approach, it represents a pretty big downside to how Prettier is currently built. Would be great to avoid this pitfall and offer extensibility from the start, if possible!
Yes, we and our students use SQL-in-JS through I'm guessing there's also other popular embedded languages where the community would benefit from their inclusion, such as XML-in-JS, YAML-in-JS, Markdown-in-JS, and other supported by If Biome doesn't want to "own" and maintain the integrations of these embedded language formatters, then an alternative would be to make it easy (easier than Prettier) for community members to integrate with custom embedded languages - potentially with an approach similar to Prettier's "Support library-specific plugin" idea (which has not yet been implemented). |
|
Oh this is a great start!
Is this commonly used? I definitively like the idea of configuring which template tag is associated to which language. We could have a default mapping like you did in your plugin. Have you some proposal to support this in the Biome config file?
This kind of variable can appear everywhere in the embedded code, it can even be part of a keyword. Even placeholder seems kind of erroneous. How Prettier is handling it? |
The initiative to support this syntax is not based on how commonly this pattern is used, but rather to provide a versatile enough approach for users to annotate the embedded code in a non-intrusive way. Only supporting tag functions will force the users to introduce runtime functions simply for dev-time formatting. Also, this VSCode extension uses block comments to add syntax highlighting support for template literals: https://marketplace.visualstudio.com/items?itemName=bierner.comment-tagged-templates
I don't have one now because I see we haven't talked about it elsewhere. But I can do some research and make a proposal later. I'm not very available in the coming days.
To be honest, I think we should limit our embedded language formatting functionality to bail out on interpolations smaller than token or node level. I would take the approach like what
Prettier simply uses carefully-constructed special identifier names as placeholders, or doesn't support interpolation for certain embedded languages:
|
I can mostly second this, and indeed I expect we'll extend our parsers to support metavariables (the token kind is already there), so it would make sense if we can reuse the same ability here. But if we accept that metavariables are the placeholders, doesn't that closely match the approach already being taken here? Maybe I forgot something since we last discussed it 😅 |
Sorry I wasn't being clear, I meant to express that we should use placeholders defined in the grammar so the parser can correctly parse that placeholder without ambiguity or choking. Currently, the placeholders used in Prettier, or my plugin |
|
@Sec-ant Thank you for your feedback, really appreciate it!
Personally I don't like the Prettier's approach, the feature shouldn't be tightly bound to some specific
Not sure if introducing extra complexity to the parser to serve the formatter is a good approach, as it seems like a form of coupling to me I have never used a |
This is to provide a general and reliable way to address the template string interpolation. For example, you use const style = css`div {
background: ${myColor};
}`;into this div {
background: biome-placeholder-0;
}which is still valid CSS syntax after transformation. But if the user write this code const style = css`div:nth-child(${myNth}) {
background: green;
}`;then after transformation it will become: div:nth-child(biome-placeholder-0) {
background: green;
}which is not syntax correct. Additionally, if we want to support more other languages for embedded language formatting in the future, a single solution like const script = js`const a = ${myValue};`const a = biome - placeholder - 0;We can of course use differet placeholders for different languages, that's also what Prettier and my plugin does. But it still leaves us with the first problem. More over, from our infrastructure, we have the ability to implement embedded language linting as well. While some of the placeholders may pass the parser, they may fail some linter rules, and the diagnostics will be confusing. So I think augmenting the grammar is the only proper way to solve this problem. And we already start implementing some kind of grammar augmentation for our plugin feature. |
Indeed! |
Yes that's basically the idea 👍. One thing I'm not sure though is the safety and future-compatibility of the |
Maybe Grit needs to introduce new syntax for querying in Sass. |
|
Oh, this is great, nice update @ah-yu ! This will indeed end up helping our CSS support for GritQL as well. Good to know: Grit actually does a clever trick under the hood, where it replaces the |
| string_interpolations.push(element.clone()); | ||
| element_iter.next(); | ||
| } | ||
| let placeholder = std::format!("biome-placeholder-{}", index); |
There was a problem hiding this comment.
Oh, and another one, - is not allowed in a gritql metavariable name: https://docs.grit.io/language/patterns#metavariables, it uses $lowercase_snake_case
| // if there are any placeholders left, we treat it is a error and format the template as normal | ||
| if !placeholder_map.is_empty() { | ||
| return Err(FormatError::SyntaxError); | ||
| } |
There was a problem hiding this comment.
I would envision use cases like the one below will be reasonable and may be common ones:
const index = 1;
const code = css`
div {
background-image: url("./assets/images/img-${index}.png");
}`;This is a sub-node level interpolation, I'm not sure whether the current replace_placeholder_with_template_element will recover such interpolations properly? I see currently we bail out if we don't restore all the interpolations from the placeholders, and format this as a normal js template string, which is good. But I guess we can keep this in mind and consider possible solutions? 🤔
There was a problem hiding this comment.
I'm not sure whether the current replace_placeholder_with_template_element will recover such interpolations properly?
Unfortunately no.
Currently, this method does an exact text match with placeholders. However, the code snippet you provided will produce a format element like text("./assets/images/img-$placeholder.png"). Maybe we can implement partial matching with placeholders and split the element into multiple segments, something like [text("./assets/images/img-"), expr, text(".png")].
There was a problem hiding this comment.
Maybe we can implement partial matching with placeholders and split the element into multiple segments, something like
[text("./assets/images/img-"), expr, text(".png")]
👍 Yes, I think we don't need an exact match.
Summary
Test Plan