Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(js_parser): support metavariables #3548

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Jul 30, 2024

Summary

Adds the ability to parse Grit metavariables to the JS parser.

I've also looked for opportunities to let the codegen take more out of our hands, but I have to admit I can't seem to find many. I did move some lexer helpers that were introduced with the CSS metavariables PR to the generic lexer.

I probably missed some places where metavariables could be allowed still, but I'd rather add those iteratively since this PR is already quite big.

Test Plan

Tests added.

@arendjr arendjr requested review from a team July 30, 2024 19:13
@github-actions github-actions bot added A-Core Area: core A-Project Area: project A-Linter Area: linter A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS labels Jul 30, 2024
Copy link

codspeed-hq bot commented Jul 30, 2024

CodSpeed Performance Report

Merging #3548 will not alter performance

Comparing arendjr:js-metavariables (eac10f7) with arendjr:js-metavariables (b2ce9dc)

Summary

✅ 104 untouched benchmarks

@Conaclos
Copy link
Member

Do you think it could make sense of using a more generic name such as JsMetaVariable instead of JsGritMetaVariable?

@arendjr
Copy link
Contributor Author

arendjr commented Jul 30, 2024

Yeah, I’m fine with that. It seems we’ll use them for other purposes too anyway.

@ematipico
Copy link
Member

ematipico commented Jul 31, 2024

What I fail to see is whether we are adding support for a specific metavariable—grit in this case—or a generic metavariable.

If we're adding support for a generic metavariable, then that's where the codegen comes into play. For example, we could define a AnyMetavariable a known node, then we could have the codegen explode this value in a better way.

The codegen will add any metavariable we will support, e.g.:

css``
html``
grit``
graphql``

And we could go even farther by filtering the metavariables based on the language. For example, if we are generating the codegen for the CSS, we shouldn't add support for the css metavariable in the language.

@arendjr
Copy link
Contributor Author

arendjr commented Jul 31, 2024

From what I understand this approach should be reusable for the metavariables inside template literals as well, so I would say it's generic. @ah-yu , correct me if I'm wrong :)

Of course it's less likely we will have JS template literals inside JS sources, which I think was the motivation the CSS parser was prepared first. So maybe this PR specifically is more beneficial towards the Grit implementation, although I expect it might even be reusable if we want to add support for HTML templates with JS snippets one day. Although that's still quite a ways out.

If we're adding support for a generic metavariable, then that's where the codegen comes into play. For example, we could define a AnyMetavariable a known node, then we could have the codegen explode this value in a better way.

I'm not really sure where an AnyMetavariable node would come into play. Maybe there's a bit of confusion going on here, since I see you mention these examples:

css``
html``
grit``
graphql``

These are not actually metavariables though, they're merely template strings within which metavariables could be used.

So what would happen for each of these template string kinds is that the respective parser gets invoked, with the option to parse metavariables enabled, but within that context you'll only need one kind of metavariable. So the CSS parser uses the CSS metavariable kind, the JS parser uses the JS metavariable kind, etc..

@arendjr
Copy link
Contributor Author

arendjr commented Jul 31, 2024

@Conaclos I will do the rename now (GritMetavariable => Metavariable) and then merge ASAP to try to avoid conflicts :)

@arendjr arendjr merged commit c9ecdac into biomejs:main Jul 31, 2024
12 checks passed
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Core Area: core A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Project Area: project A-Tooling Area: internal tools L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants