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

Prebuilt for Electron and Node #106

Merged
merged 4 commits into from
May 10, 2019
Merged

Conversation

rebornix
Copy link
Contributor

@rebornix rebornix commented May 2, 2019

node-tree-sitter is built against all ABI versions, we may want to do the something for languages otherwise we don't have rebuilds for pure Nodejs.

[node-tree-sitter](https://github.com/tree-sitter/node-tree-sitter/blob/master/package.json#L30) is built against all ABI versions, we may want to do the something for languages otherwise we don't have rebuilds for pure Nodejs.
@rebornix
Copy link
Contributor Author

rebornix commented May 2, 2019

I think the .travis.yml should be updated as well to pass the build, @maxbrunsfeld any reason why it differs from https://github.com/tree-sitter/node-tree-sitter/blob/master/.travis.yml ?

@rebornix rebornix changed the title Build for all supported ABI versions Prebuilt for Electron and Node May 9, 2019
@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented May 10, 2019

We had it this way originally, but our Travis CI builds were timing out, because compiling this library takes longer than compiling node-tree-sitter. We need to limit the builds to only use the versions that we really need.

I would suggest maybe keeping Node 8 and 10 (not 9), and not adding Electron 1.6 and 1.7.

@maxbrunsfeld
Copy link
Contributor

@rebornix Just in case you're interested:

Another potential solution to this problem would be for vscode-ruby to switch to using the WebAssembly binding to Tree-sitter. It has almost exactly the same API as node-tree-sitter, and you could completely avoid the issue of ABI/platform differences. You can demo the Ruby parser in the browser here, and there's a bit more info here.

The only downsides I'm aware of are:

  • Very old node versions may not have Wasm support (not sure when it landed)
  • Parser and Tree instances need to be explicitly deleted by calling .delete(), because WebAssembly won't have GC callbacks until the weakref proposal lands.
  • The WebAssembly binding is newer and may have bugs I haven't seen

@maxbrunsfeld maxbrunsfeld merged commit 624487a into tree-sitter:master May 10, 2019
@rebornix
Copy link
Contributor Author

rebornix commented May 10, 2019

@maxbrunsfeld thanks for the quick response! wasm is a decent solution indeed and considering that the majority of VS Code users are using latest few versions of Electron, wasm can be supported nicely.

For native module prebuilt, do you think we can move to N-API from NAN, then we don't need to worry about Node headers. If we target napi version 3 and then we can support Node.js 8.9 and above. For example, I converted node-spellchecker to napi-spellchecker, which solved this issue in another way.

@rebornix rebornix deleted the patch-1 branch May 10, 2019 16:28
@maxbrunsfeld
Copy link
Contributor

I would love to switch to NAPI. It seems like a fair amount of work, as node-tree-sitter uses a fair number of different V8/node features like async workers, weak references, string manipulation, etc.

If you're interested in opening a PR to convert it, that'd be amazing.

@rebornix
Copy link
Contributor Author

sure, I'll play with both tree-sitter-ruby and node-tree-sitter to see how easy it is to do the migration and agree, having v8 dependency makes it hard.

@wingrunr21
Copy link
Contributor

@maxbrunsfeld I would be open to switching to the WASM version but I'd like to do it when we are comfortable in its implementation. It would make embedding this in the extension much easier.

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.

3 participants