-
-
Notifications
You must be signed in to change notification settings - Fork 146
Keep API in-line with upstream #163
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
Conversation
|
One function that's sorely missing is maybe some of these are also missing functions that should have bindings. I generated it like this: cd /tmp
git clone https://github.com/tree-sitter/node-tree-sitter
rg -o --no-filename 'ts_\w*\(' node-tree-sitter/src | sort | uniq > used_in_node.txt
curl https://raw.githubusercontent.com/tree-sitter/tree-sitter/master/lib/include/tree_sitter/api.h | rg -o --no-filename 'ts_\w*\(' | sort | uniq > api.h
comm -23 api.h used_in_node.txtThanks for working on this. |
|
Btw, do we want to switch to napi still? I'd like to do that upstream and here if that proves to be very beneficial, I forgot what exactly the benefits are rn |
|
The benefits are that you don't need to upload as many binary files with each node-tree-sitter release, there's less work to get it to support each new Node version and something about making the extension "context aware" might be easier. And it's a newer API. It would be good to do |
|
Agreed, it's just massively breaking as all grammars will need to regenerate their bindings to have the napi one instead of nan |
|
I'm nearing the finish line for this PR (pretty much just LookaheadIterator left) - but that being said, I have no idea why CI is failing, it does require rebuilding the bindings instead of using the latest release for obvious reasons... |
7bc63be to
c622614
Compare
|
LookaheadIterator is done - the only functions that are available upstream but not used here are the following: Some are just not needed, and those related to language need changes upstream to allow the generated tree-sitter-xxx binding to expose these, as that contains the "Language" type, and isn't created here. We could create an extended method to say, call Tests are done, I just have to update the dsl/index.js and then this is ready |
|
alright it's done, do try it out thoroughly if you can |
| captures(rootNode: SyntaxNode, startPosition?: Point, endPosition?: Point): QueryCapture[]; | ||
| reset(language: any, state: number): boolean; | ||
| resetState(state: number): boolean; | ||
| next(): number; |
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.
I get this type error trying to use this code:
../third-party/node-tree-sitter/tree-sitter.d.ts:214:7 - error TS2416: Property 'next' in type 'LookaheadIterator' is not assignable to the same property in base type 'IterableIterator<number>'.
Type '() => number' is not assignable to type '(...args: [] | [undefined]) => IteratorResult<number, any>'.
Type 'number' is not assignable to type 'IteratorResult<number, any>'.
214 next(): number;
~~~~
If I change this to
| next(): number; | |
| next(): IteratorResult<number>; |
then it complains that
../third-party/node-tree-sitter/tree-sitter.d.ts:208:18 - error TS2420: Class 'LookaheadIterator' incorrectly implements interface 'IterableIterator<number>'.
Property '[Symbol.iterator]' is missing in type 'LookaheadIterator' but required in type 'IterableIterator<number>'.
208 export class LookaheadIterator implements IterableIterator<number> {
~~~~~~~~~~~~~~~~~
node_modules/typescript/lib/lib.es2015.iterable.d.ts:53:5
53 [Symbol.iterator](): IterableIterator<T>;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
'[Symbol.iterator]' is declared here.
Found 1 error in ../third-party/node-tree-sitter/tree-sitter.d.ts:208
|
superseded by #190 |
Similar to py-tree-sitter's updates, not done yet but putting it up for early bird reviewers:
Todo:
@verhovsky you might be interested