cursorless-dev / vscode-parse-tree

Syntax trees for VSCode using tree-sitter
MIT License
40 stars 35 forks source link

Bundle web-tree-sitter and added Haskell support #20

Closed wenkokke closed 2 years ago

wenkokke commented 2 years ago

This pull request changes the build process of vscode-parse-tree to build web-tree-sitter, rather than depending on the published version in the NPM package registry, as it frequently lags behind the main version.

Furthermore, it uses this to add support for Haskell.

wenkokke commented 2 years ago

This seems to have loading path issues?

wenkokke commented 2 years ago

How come the yarn.lock keeps getting bumped? Are you explicitly asking it to upgrade everything?

I don't know, to be honest?

wenkokke commented 2 years ago

I've merged haskell-support into this pull request.

wenkokke commented 2 years ago

This is the diff for Haskell support: https://github.com/pokey/vscode-parse-tree/pull/20/commits/a5f6a921c034df0f92e7e501136d8e55fdb1735e

pokey commented 2 years ago

Is this one ready for another round of review? If not, of course feel free to re-request review when you want me to have another look

wenkokke commented 2 years ago

It is :)

pokey commented 2 years ago

Ok cool looks good, but I'll need to try it out locally. One last thing from my previous review:

I'm not sure I understand what the workflow is supposed to be now, though. Eg if I just clone the repo and type yarn, that won't work, right, because I haven't generated the vendor/web-tree-sitter dir yet? But if I run yarn compile, that won't work either, because I haven't installed anything. So it seems like there's a bit of a chicken and egg thing. But I'm probably missing something

wenkokke commented 2 years ago

I don’t use yarn, so I’m not sure how it differs from npm, but npm run compile calls make compile, which generates the required vendor/web-tree-sitter/[VERSION]/ dependency.

pokey commented 2 years ago

I don’t use yarn, so I’m not sure how it differs from npm, but npm run compile calls make compile, which generates the required vendor/web-tree-sitter/[VERSION]/ dependency.

Hmm I cloned a fresh repo on your branch, and did an npm run compile (or rather, a yarn compile, which is the same), and got the following:

make: *** No rule to make target `parsers/tree-sitter-agda.wasm', needed by `compile'.  Stop.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I believe that's because the dependency of parsers/tree-sitter-agda.wasm is node_modules/tree-sitter-agda/package.json, but that doesn't yet exist because nothing has been installed yet, as I haven't yet run yarn (equivalent to npm install).

If, however, I instead start by running yarn, I get the following error:

➜ yarn                 
yarn install v1.22.17
warning ../../package.json: No license field
[1/5] 🔍  Validating package.json...
warning parse-tree@0.14.0: The engine "vscode" appears to be invalid.
[2/5] 🔍  Resolving packages...
error Package "web-tree-sitter" refers to a non-existing file '"/Users/pokey/src/vscode-parse-tree-test/vendor/web-tree-sitter/v0.20.4"'.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Because I haven't yet run yarn compile. That's what I meant by a chicken-and-egg thing. You need to compile to create the proper dependencies, but then you need the proper dependencies to compile

I think the reason that things are working for you is that you're not in a fresh directory; you already have the previously installed packages in your node_modules directory

I think we probably need to split the makefile's compile rule into two separate rules: one to build the web-tree-sitter package, and one to compile. You would run the first rule, then run yarn, then run the second rule. Make sense?

Would also be great if you could do a quick update to the docs to this effect

wenkokke commented 2 years ago

Changes:

wenkokke commented 2 years ago

Ok so this all looks great! +70 for persistence 😅. I updated the contributing instructions; they haven't really changed it seems but I find it works better if I run yarn compile so that emscripten ends up on the path. A couple things left:

  • [ ] Would be good to update the docs with instructions on how to upgrade web-tree-sitter, ie: that patch file, etc
  • [ ] I'm still getting those failures on the cursorless html test cases after installing this branch locally. If you're unsure how to debug the cursorless side of things I'd be happy to pair
stack trace: TypeError: Cannot read property 'apply' of undefined
  at [/Users/pokey/.vscode/extensions/pokey.parse-tree-0.14.0/node_modules/web-tree-sitter/tree-sitter.js:1:10374]()
  at _ZN3TagC2E7TagTypeRKNSt3__212basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE (<anonymous>:wasm-function[141]:0xad34)
  at _ZN3Tag8for_nameERKNSt3__212basic_stringIcNS0_11char_traitsIcEENS0_9allocatorIcEEEE (<anonymous>:wasm-function[125]:0xaa8e)
  at tree_sitter_html_external_scanner_scan (<anonymous>:wasm-function[25]:0x9d5e)
  at ts_parser_parse_wasm (<anonymous>:wasm-function[214]:0x24857)
  at Parser.parse ([/Users/pokey/.vscode/extensions/pokey.parse-tree-0.14.0/node_modules/web-tree-sitter/tree-sitter.js:1:35040]())
  at [/Users/pokey/.vscode/extensions/pokey.parse-tree-0.14.0/out/extension.js:82:43]()
  at Generator.next (<anonymous>)
  at fulfilled ([/Users/pokey/.vscode/extensions/pokey.parse-tree-0.14.0/out/extension.js:5:58]())
  at runMicrotasks (<anonymous>)
  at processTicksAndRejections (internal[/process/task_queues.js:93:5]())

How do I reproduce this HTML error?

pokey commented 2 years ago

You should be able to do a local package and install of this extension, then clone cursorless and run cursorless test suite locally

Actually you'll probably want to clone and run cursorless tests locally first, just to make sure they're working for you normally 😊

Again, happy to pair up here if helpful

pokey commented 2 years ago

Ok as discussed this one now seems to be working for me with cursorless locally, so just docs on how to upgrade web-tree-sitter and I think we're good to go!

wenkokke commented 2 years ago

I have added a short section documenting how to update web-tree-sitter, and I've added the version number to the patch file so that we can store some historical patches if we need to, and to make the upgrade process easier, e.g., no need for creating empty patches or for changing the patching code.

ParetoOptimalDev commented 2 years ago

@wenkokke Are you also working on support for Haskell in cursorless-dev/cursorless-vscode?

If not I might look in too starting since cursorless could make my work week a lot easier :smile:

wenkokke commented 2 years ago

Yes, that’s next on my calendar, but if you’d like to have a chat I’d be happy to!