search

tree-sitter / node-tree-sitter

Node.js bindings for tree-sitter
https://www.npmjs.com/package/tree-sitter
MIT License
649 stars 114 forks source link

Support Node 19 and 20 #127

Closed verhovsky closed 1 year ago

verhovsky commented 1 year ago

and also update the dependencies.

Closes #126 , closes #90 and closes https://github.com/tree-sitter/tree-sitter/issues/1945 (which should have been opened on this repo)

verhovsky commented 1 year ago

With these changes I was able to npm install on macOS (ARM) with Node 14 and Node 18, although npm test errors out with

RangeError: Incompatible language version. Compatible range: 13 - 13. Got: 14
verhovsky commented 1 year ago

After updating vendor/tree-sitter I'm able to run npm test too.

verhovsky commented 1 year ago

I couldn't figure out how to fix the CI with Windows, the last thing I tried is setting the Node version to 16 and it failed to download node to install it

npm ERR! gyp ERR! stack Error: 500 response downloading https://nodejs.org/download/release/v16.18.1/node-v16.18.1-headers.tar.gz

So I replaced the Windows with Ubuntu instead in the CI and it works... anyway, we should test Windows with #128

jgresty commented 1 year ago

Keen to have this merged as the currently used version of prebuild-install is vulnerable to CVE-2021-3807 through its dependencies. Fixed in 7.1.1

verhovsky commented 1 year ago

@jgresty inefficient regexes are CVE spam, not real vulnerabilities. If prebuild-install has an inefficient regex it shouldn't matter anyway because it runs once when you install the package.

The bigger problem and the reason this should really be merged/deployed is that packages that use tree-sitter-node have never worked on Node 19, which was released 5 months ago and is the default version that gets installed on macOS when you do brew install node. @maxbrunsfeld could you please merge/deploy this? It's good to go, the only thing left is the superstring dependency. You should fork it if you're not able to publish new versions for their package on npm anymore. I can also do it if you'd like. I published a fork as https://www.npmjs.com/package/@curlconverter/superstring but I would prefer to update the upstream or at least have you able to release new versions of the npm package.

jaisnan commented 1 year ago

Is there any reason this isn't merged yet?

NikkTod commented 1 year ago

Could you please merge 👍

cclauss commented 1 year ago

@NikkTod Please

  1. Click on https://github.com/tree-sitter/node-tree-sitter/pull/127/files
  2. Click the Review changes button
  3. Click Approve
  4. Click Submit review.
tthornton3-chwy commented 1 year ago

@verhovsky thanks so much for working on this! looks like we got the approvals :) Could we try and merge this in, this dependency trickles its way on up to a bunch of other places that are currently borked because of it after moving to Node 20, and fixing it here at the source is a lot better of a solution :D

TylerLeonhardt commented 1 year ago

👋 I think maybe @maxbrunsfeld is maybe the best person to (gently) ping? I'm coming from this tree-sitter-typescript issue. Would love to see Node 19+ working with node-tree-sitter 🙏 so folks can build VS Code extensions with newer versions of Node.

cclauss commented 1 year ago

Please change the title of this PR to add 18 and remove 19 which is EOL.

verhovsky commented 1 year ago

I split this PR into separate commits: #134 #140 #141 #142