-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Syntax and language configuration fixes and improvements #286
Conversation
🦋 Changeset detectedLatest commit: 1154e1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -3,7 +3,6 @@ | |||
"blockComment": ["<!--", "-->"] | |||
}, | |||
"brackets": [ | |||
["---", "---"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the frontmatter bracket because it causes an indent bump into the template (since the entry and the exit of the bracket are the same characters). If we want to provide an indent into the frontmatter, we should use entry rules
…h different types
@@ -5,8 +5,7 @@ | |||
"brackets": [ | |||
["<!--", "-->"], | |||
["<", ">"], | |||
["{", "}"], | |||
["(", ")"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no cases where you want parenthesizes to count as brackets in the source.astro
scope, in contexts where they do matter (ex: source.ts
, source.js
, source.tsx
) the languages used there will add them as brackets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic! None of these comments are blocking, just curious to hear your thoughts.
{ "open": "{/*", "close": " */}", "notIn": ["comment", "string"] }, | ||
{ "open": "<!--", "close": " -->", "notIn": ["comment", "string"] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that the close has a leading space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! For comments it makes for a nicer experience since people usually add a space at the end. This is actually a consistency fix, we already had the space for JS comments but not HTML ones
"indentationRules": { | ||
"increaseIndentPattern": "<(?!\\?|(?:area|base|br|col|frame|hr|html|img|input|keygen|link|menuitem|meta|param|source|track|wbr)\\b|[^>]*\\/>)([-_\\.A-Za-z0-9]+)(?=\\s|>)\\b[^>]*>(?!.*<\\/\\1>)|<!--(?!.*-->)|\\{[^}\"']*$", | ||
"decreaseIndentPattern": "^\\s*(<\\/(?!html)[-_\\.A-Za-z0-9]+\\b[^>]*>|-->|\\})" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this will be great! Nice job
}, | ||
{ | ||
"name": "meta.tag.component.astro", | ||
"begin": "(</?)([$A-Z_][^/?!\\s<>]*|[^/?!\\s<>.]+\\.[^/?!\\s<>]+)\\b", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unchanged, but just want to confirm that namespace.component
is properly highlighted as a component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, what gives it the special highlighting is actually the support.class.component.astro
scope. Since it makes it highlighted like a class (in TSX files, it's support.class.component.tsx
)
"comment": "Treat script tag with application/ld+json as JSON. This needs to be higher than is:raw since it's a very possible situation to have is:raw on a JSON script tag", | ||
"begin": "\\G(?=\\s*[^>]*?type\\s*=\\s*(['\"]|)(?i:application/ld\\+json)\\1)", | ||
"end": "(?=</|/>)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love to see it!
] | ||
}, | ||
{ | ||
"comment": "Treat script tag with is:raw as plain text", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about the highlighting here... I don't think is:raw
has any effect on script
or style
(braces are already ignored), so do we really want highlighting to change to plaintext? I think everything will still be parsed and bundled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I might have misunderstood what is:raw
does on those tags then!
I thought it would act like a more intense is:inline
basically. Especially for script tags, I felt like having a weird type + is:raw
could be a "common" case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to remove is:raw
as plaintext for style tags and script tags after confirming that it doesn't do anything in Astro
} | ||
} | ||
|
||
snapShotTest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests look awesome!
Changes
This fixes a few issues with our syntax file and our language configuration file and also adds some missing features:
is:raw
on script tags, style tags, components and other elementsapplication/ld+json
(I think this makes us one of the rare ones to support this, I thought it was neat and fairly easy so, why not){/* */}
Fix #7
Fix #112 however, auto indent is super hard to get right and very flaky. Even in HTML and TSX files it doesn't always works very nicely. I ended up mostly copying the TSX's auto indent rules with a few tweaks from the HTML ones
Fix #274 - It doesn't really fix it but making this PR made me realize that it's not a really possible thing to do the way I thought we could do it when I wrote that issue and that things worked differently than I thought, so it's no longer relevant
Half of #285
I tried to do #271, but I couldn't figure it out. Including the
html:comment
pattern inside expressions worked in some cases but didn't in others and I don't really get why. I then tried to inject HTML comments inside the JSX grammar and ended up with nothing more than an headache. If someone has a solution, I'll gladly take it ha!Testing
Added a test suite powered by vscode-tmgrammar-test with instructions on how to use and when to update the snapshots
Docs
No docs needed?