-
-
Notifications
You must be signed in to change notification settings - Fork 843
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(ecma): inject template methods #6555
base: master
Are you sure you want to change the base?
feat(ecma): inject template methods #6555
Conversation
(member_expression | ||
property: (property_identifier) @injection.language) |
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 kindof open a weird can of wormholes. What about foo.bar.sql
, foo.bar.baz.sql
, sql.bar
, ....
I'd prefer to keep this in my own config, but not in nvim-treesitter.
Unless you can give clear reasonings why this won't lead to the can of worms above
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.
That's a fair point. Here's my reasoning for what it's worth:
The foo.sql
pattern appears in one of the leading Javascript frameworks SQL SDKs (see Vercel's PostgreSQL SDK). This makes me feel that it is common enough to be covered here instead of in each user’s individual config (similar to how we already cover things like Styled Components via the styled.div
injection).
On the other hand, I can’t think of a non user-constructed scenario (or any library) that would have a pattern like foo.bar.sql
, foo.bar.baz.sql
, sql.bar
. This makes me feel that these are niche/rare enough to not be covered here but instead in a given user’s config.
While this doesn’t address why this wouldn’t open a can of worms, it does address why we might not feel compelled make any further changes related to it.
If you prefer the route of covering specific libraries (similar to Styled Components) let me know and I can amend this to have a (potentially separate) query only covering sql
or even just client.sql
.
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.
I will go for only covering for the most popular (like top 500 node packages).
Vercel's Postgres is popular enough so I'd give it a pass. Not sure about other cases though.
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.
Sounds good. Are you thinking of walking back the changes except for that to the query that covers sql or would you rather I create a new query for just sql that covers this foo.sql
case specifically?
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.
Preferably writing a new query covering whatever that vercel postgres will be for + comments close to the new query
Added injection into tagged templates for method calls (e.g.,
client.sql`...`
) as mentioned in #6550.