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

Support new curly brace syntax #43

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

kevinschweikert
Copy link
Contributor

Closes #42

This is me just jumping into the codebase but i wanted have a stab at it. Hope this helps and i'm happy to get feedback if anything needs changing!

@connorlay
Copy link
Member

Thanks for doing this! I'll have time tomorrow to test these changes locally.

grammar.js Outdated
Comment on lines 134 to 146
seq(
choice("{"),
prec.left(
seq(
choice(
$.partial_expression_value,
$.ending_expression_value,
$.expression_value
),
choice("}")
)
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial/end expression is not allowed in {...}, it should always be a single expression. We can do this:

Suggested change
seq(
choice("{"),
prec.left(
seq(
choice(
$.partial_expression_value,
$.ending_expression_value,
$.expression_value
),
choice("}")
)
)
)
alias($.expression, "expression")

Aliasing as string makes the node anonymous, so it appears as directive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I would consider parsing {...} as expression node, regardless if it's <div attr={...}> or <div>{...}</div>. So it would be this:

_node: ($) =>
-      choice($.doctype, $.tag, $.component, $.text, $.comment, $.directive),
+      choice($.doctype, $.tag, $.component, $.text, $.comment, $.directive, $.expression),

@connorlay wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonatanklosko That makes sense to me!

parse curly braces directive only with single expressions

Co-authored-by: Jonatan Kłosko <[email protected]>
@SteffenDE
Copy link

Is tree-sitter able to respect phx-no-curly-interpolation? It might not be worth it, but I tried to do it for vscode with Textmate Grammars but then realized they cannot support it :(

@jonatanklosko
Copy link
Contributor

Is tree-sitter able to respect phx-no-curly-interpolation?

I think it may be possible, but I'm pretty sure it's not worth the complexity.

The way I would try to implement it, would be to have a separate grammar node for phx-no-curly-interpolation and two variants of tags (one with phx-no-curly-interpolation and one without). Since the tags are recursive, once a tag has phx-no-curly-interpolation we need to respect that for all children, so we would basically have to duplicate a whole chunk of the grammar.

Copy link
Member

@connorlay connorlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally got time to test this myself and it looks great! Thanks everyone!

@connorlay connorlay merged commit f3d5585 into phoenixframework:main Dec 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new curly braces syntax
4 participants