-
Notifications
You must be signed in to change notification settings - Fork 107
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 Node 19 and 20 #127
Conversation
package.json
Outdated
"mocha": "^10.2.0", | ||
"node-gyp": "9.3.1", | ||
"prebuild": "^11.0.4", | ||
"superstring": "https://github.com/pulsar-edit/superstring#node-api-upgrade", |
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 is not great. superstring is deprecated and it has the same issue that it uses the same V8 functions that don't exist anymore. The changes in this fork were proposed upstream atom/superstring#94 but they're not going to get merged of course. Can we remove superstring as a tree-sitter dependency?
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.
We could also fork and publish as @tree-sitter/superstring
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.
Actually, since superstring
is in devDependencies
, we shouldn't strictly need to fix it to make tree-sitter
installable on Node 19 or 20, so I undid this change.
"@types/node": "^18.14.6", | ||
"chai": "^4.3.7", | ||
"mocha": "^10.2.0", | ||
"node-gyp": "9.3.1", |
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 isn't strictly necessary but it lets you run the project with Python 3.11
With these changes I was able to
|
After updating vendor/tree-sitter I'm able to run |
afa680a
to
8973bd6
Compare
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
So I replaced the Windows with Ubuntu instead in the CI and it works... anyway, we should test Windows with #128 |
@@ -34,7 +34,7 @@ void InitConversions(Local<Object> exports) { | |||
v8::Local<v8::Object> bufferView; | |||
bufferView = node::Buffer::New(Isolate::GetCurrent(), point_transfer_buffer, 0, 2 * sizeof(uint32_t)).ToLocalChecked(); | |||
auto js_point_transfer_buffer = node::Buffer::Data(bufferView); | |||
#elif V8_MAJOR_VERSION >= 8 | |||
#elif (V8_MAJOR_VERSION > 8 || (V8_MAJOR_VERSION == 8 && V8_MINOR_VERION > 3)) |
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.
Introduced in 8.3.9 v8/v8@5cf02f0
Keen to have this merged as the currently used version of |
This comment was marked as outdated.
This comment was marked as outdated.
Node 19 is [not supported by tree-sitter yet](tree-sitter/node-tree-sitter#127), and that is a dependency of `@fastly/js-compute`. Until tree-sitter works on Node 19, we should not list Node 19 as supported.
Node 19 is [not supported by tree-sitter yet](tree-sitter/node-tree-sitter#127), and that is a dependency of `@fastly/js-compute`. Until tree-sitter works on Node 19, we should not list Node 19 as supported.
Node 19 is [not supported by tree-sitter yet](tree-sitter/node-tree-sitter#127), and that is a dependency of ours. Until tree-sitter works on Node 19, we should not list Node 19 as supported.
Node 19 is [not supported by tree-sitter yet](tree-sitter/node-tree-sitter#127), and that is a dependency of ours. Until tree-sitter works on Node 19, we should not list Node 19 as supported.
Node 19 is [not supported by tree-sitter yet](tree-sitter/node-tree-sitter#127), and that is a dependency of `@fastly/js-compute`. Until tree-sitter works on Node 19, we should not list Node 19 as supported.
Is there any reason this isn't merged yet? |
Could you please merge 👍 |
@NikkTod Please
|
@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 |
👋 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. |
@@ -196,7 +196,7 @@ void Tree::GetEditedRange(const Nan::FunctionCallbackInfo<Value> &info) { | |||
|
|||
void Tree::PrintDotGraph(const Nan::FunctionCallbackInfo<Value> &info) { | |||
Tree *tree = ObjectWrap::Unwrap<Tree>(info.This()); | |||
ts_tree_print_dot_graph(tree->tree_, stderr); | |||
ts_tree_print_dot_graph(tree->tree_, 2); |
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 happens because of updating vendor/tree-sitter:
../src/tree.cc:199:3: error: no matching function for call to 'ts_tree_print_dot_graph'
ts_tree_print_dot_graph(tree->tree_, stderr);
^~~~~~~~~~~~~~~~~~~~~~~
../vendor/tree-sitter/lib/include/tree_sitter/api.h:423:6: note: candidate function not viable: no known conversion from 'FILE *' (aka '__sFILE *') to 'int' for 2nd argument
void ts_tree_print_dot_graph(const TSTree *, int file_descriptor);
^
introduced in tree-sitter/tree-sitter@97fd990
Please change the title of this PR to add |
and also update the dependencies.
Closes #126 , closes #90 and closes tree-sitter/tree-sitter#1945 (which should have been opened on this repo)