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 <template> tags for Glimmer #208

Merged
merged 2 commits into from
Aug 18, 2022
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 3, 2021

Supports <template> tags for Glimmer

Checklist:

  • All tests pass in CI. -- waiting on C.I. approval
  • The script/parse-examples passes without issues.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

Images of what this enables (+the injected grammar (glimmer))
image
image
image

Originally, I thought I could "just use injections" to hijack JSX parsing to give the <template> tags different highlighting -- this doesn't work tho because tree-sitter-javascript is still going to parse everything as JSX (even if there is an additional highlighting injection which parses as glimmer (via tree-sitter-glimmer). This created TOOOOONS of ERROR nodes, and weird partially recovered nodes in the tree.
The easiest path forward here was to add <template> parsing and allow tree-sitter-glimmer to take over from there.

One thought I had, which I feel like keeps coming up throughout all of JS is that it seems integration JSX directly into JS parsers leads to soft-blocking other tools that want to use similar syntax -- a design we may want to consider in the future is removing JSX from base-javascript parsing (but providing tools to match on <...> and </...> (and <>/</>), so that separate parsers can then highlight the contents. This would make things a bit more modular, and would support JSX, TSX, Glimmer, Svelte, etc all as separate packages that can use injections for the content within the <...> nodes. Though, even though the modularity of this idea is a bit exciting to me, the amount of effort to migrate things that way feels enormous (esp dealing with the social / semver contracts, etc)

@NullVoxPopuli
Copy link
Contributor Author

@mjambon / @maxbrunsfeld

I could use some help, if ya'll'd be so kind <3

  • how do I run the tests?
  • do I need to compile something?
  • what tools do I need in order to compile?
  • what else do I need to know if I've never contributed to tree-sitter before?

@mjambon
Copy link
Contributor

mjambon commented Dec 17, 2021

@NullVoxPopuli we should create a CONTRIBUTING.md document with answers to these questions. I don't know if there's one already for one the other tree-sitter-xxx projects that we could reuse. @maxbrunsfeld , do we have this? If not, I'll start one that can be reused for similar projects.

See issue #213

@mjambon
Copy link
Contributor

mjambon commented Dec 17, 2021

@NullVoxPopuli you need the tree-sitter executable (rust binary). The easy way for many people for this is to use npm although it's not required. You'd do this:

npm install
npm run test

With the above, the tree-sitter binary gets installed somewhere in node_modules and you can call it with the npx command e.g. you can run npx tree-sitter generate. I'm still not comfortable with node/npm ecosystem. I suppose you have inspect package.json to figure out the different actions available.

The more direct install, which I prefer, consists in installing tree-sitter from the rust source somewhere in my PATH, then use these commands:

tree-sitter generate  # compile the parser
tree-sitter test  # run the test suite

For parsing just a file, use this:

tree-sitter parse foo.ext

For parsing a snippet when using bash, I recommend using process substitution so you don't have to create a file:

tree-sitter parse <(echo 'const x = 42;')

FWIW, this is the script we use to install the tree-sitter executable for ocaml-tree-sitter and this is the script we use to install the runtime library (libtree-sitter.so, libtree-sitter.a).

@NullVoxPopuli
Copy link
Contributor Author

how do you edit the js parser? it's 74k lines and is too much for my editor 😅

🤔

I'm wanting to "change languages" when I encounter a <template> keyword (and resume JS/TS after the matching </template>

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jul 3, 2022

ok, I'm back at this -- I've pushed up some tests for kind of what I'm expecting to happen.

Is there a way to setup tree-sitter for testing that injections are working appropriately? I noticed that for JSDoc, the whole thing is just "comment".

in the TS Playground, I can toggle injections and then I see

comment [0, 0] - [2, 3]
  tag [1, 3] - [1, 17]
    tag_name [1, 3] - [1, 10]
    type [1, 12] - [1, 16]
function_declaration [3, 0] - [3, 17]
  name: identifier [3, 9] - [3, 12]
  parameters: formal_parameters [3, 12] - [3, 14]
  body: statement_block [3, 15] - [3, 17]

for

/*
 * @return {void}
 */
function foo() {}

My hope is that I can at least test that the injection is supposed to happen (and then do the same for the glimmer injections) -- (I understand that if we were to test full AST of all injections, we'd have circular dependencies, and that'd get wonky)

@NullVoxPopuli
Copy link
Contributor Author

I'm starting to think this isn't possible without changing parser settings?

The main thing I'm running into as that entire <template> blocks are registered as Errors -- because if this was JSX, it would be.

Unrelated, or maybe it is related -- but could have JSX been a separate parser / syntax with injections into javascript? if that's theoretically possible, than so should this Glimmer stuff 🤔

@NullVoxPopuli
Copy link
Contributor Author

This best I've been able to do so far is double parsing.

image

where the same text is parsed by both JSX and Glimmer. 🤔
Is there a way to turn JSX parsing off when another language takes over?

@NullVoxPopuli
Copy link
Contributor Author

I think the problem, If I'm understanding correctly, is that injections only control highlighting.. they can't stop parsing from the host language -- which is what I think I need to have happen.

@NullVoxPopuli NullVoxPopuli force-pushed the glimmer branch 3 times, most recently from ef8d87b to 2efc7dc Compare July 4, 2022 17:08
@NullVoxPopuli
Copy link
Contributor Author

@maxbrunsfeld, @mjambon ready for review

@NullVoxPopuli NullVoxPopuli changed the title Add ember/glimmer/handlebars/etc injections Support <template> tags for Glimmer Jul 4, 2022
queries/injections.scm Outdated Show resolved Hide resolved

; Ember Unified <template> syntax
; e.g.: <template><SomeComponent @arg={{double @value}} /></template>
((glimmer_template) @glimmer)
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Jul 4, 2022

Choose a reason for hiding this comment

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

I got it!

image

I figured out how to test this locally!

working great!
(and now further highlight token can be created in tree-sitter-glimmer)

@NullVoxPopuli
Copy link
Contributor Author

bump <3

@NullVoxPopuli
Copy link
Contributor Author

@maxbrunsfeld, may I request a review?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Aug 17, 2022

Bump

@mjambon ? <3

</template>;

<template>
<WithSemi />
Copy link
Contributor

Choose a reason for hiding this comment

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

So whatever is inside the is not really parsed/recognized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, that's handled by tree-sitter-glimmer via injections

@aryx
Copy link
Contributor

aryx commented Aug 18, 2022

I'd rather see this outside base-javascript, but jsx is already in, so why not glimmer.
Seems like JSX parses more deeply things, but maybe this can be added for glimmer in further PRs.

@aryx aryx merged commit 3dac6b0 into tree-sitter:master Aug 18, 2022
@NullVoxPopuli
Copy link
Contributor Author

I'd rather see this outside base-javascript, but jsx is already in,

I'd love to see a movement more where jsx (and other flavors) are additions, rather than builtin -- makes more sense architecturally, I agree <3

@sandstrom
Copy link

Awesome @NullVoxPopuli 🎉

@NullVoxPopuli
Copy link
Contributor Author

aaand typescript update: tree-sitter/tree-sitter-typescript#218

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.

4 participants