Skip to content

Conversation

@branchvincent
Copy link
Contributor

Hey, cool project!

This swaps the wasm bindings for native node bindings, fixing #17. Some things to note:

@ndonfris
Copy link
Owner

ndonfris commented Jan 8, 2025

Hello! Awesome work here :)

My primary motivation for opening that discussion was because of the tree-sitter documentation which states:

Notice that executing .wasm files in node.js is considerably slower than running node.js bindings.

Unfortunately, upon further investigation, I did not find this to be case. Directly comparing these two dependencies locally, I found that the node bindings were significantly slower than the wasm bindings

wasm using web-tree-sitter node using node-tree-sitter
720 fish files
{functions,conf.d}/*.fish, config.fish
692 fish files
functions/*.fish, config.fish
Elapsed time: 1.69s Elapsed time: 3.53s
web-tree-sitter node-tree-sitter
node version 22.12.0 node version 22.12.0

Please feel free to compare the startup timing yourself. Relevant change can be found here in the src/analyze.ts file.

note: that depending on your language client, and the value of set -gx fish_lsp_show_client_popups, the startup time might only be shown in your set -gx fish_lsp_logsfile /tmp/fish-lsp.logs file.

IMO, I think the performance difference is a little too large currently, and I'm not sure if it's worth the tradeoff as the web-tree-sitter and node-tree-sitter packages appear to be pretty noticeably inconsistent in what features they support. Indexing the completions/*.fish files is planned in v1.0.9, and that'd probably double the amount of files indexed, at least.

If we do plan on adopting the node bindings, I think it would be worth further documenting what differences there are between the two packages, and how to handle them in the future. The eventually 1.0.9 uses a lot of the SyntaxNode class, so switching to the node bindings will likely require a lot of changes to upstream branches (as seen on both PR #52 and the dirty branch fix/def-scope-extended, with the FishSymbol changes)

For now I'll leave this open, and continue looking into using the node bindings over wasm. Feel free to continue the discussion here, or on the old discussion. It's really unfortunate that the node bindings are so slow, as having features like typedocs and easier testing would be really nice to have.


On some of the other fixes you mentioned:

  • I cleaned up some other deps, removing unused tsc (this was superseded by typescript), chai, tree-sitter-cli

  • I dropped the preinstall script to allow installing with npm, which partially fixes Why is yarn required for the build? #48 and NPM Install Fail #57 (scripts/relink-locally.fish still requires yarn but I have some suggested packaging changes that I'll make in a separate PR to fully fix)

Please feel free to open a PR for this, I'd be happy to help you get it merged.

I've been meaning to finalize v1.0.8-3 for a while now, and I think this would be a great addition to that release. Here's my current plan for resolving the package manager issues.

Thanks again for such a thorough PR! Hoping we can merge it eventually.

ndonfris added a commit that referenced this pull request Jan 13, 2025
ndonfris added a commit that referenced this pull request Jan 14, 2025
* feat: added install script and updated docs

* docs: fixups to README

* docs: added edit_command_buffer to README.md

* fix: `fish-lsp info` logsfile location & readme wording

* fix: `package.json` removed linking, reworded setup section `README.md`

* fix: `package.json` pkg config

* fix: audit `yarn.lock` removed deprecated `cross-spawn`

* fix: removed unused fish-lsp enviornment variables w/ README changes

- removed `fish_lsp_format_tabsize` & `fish_lsp_format_switch_case`
- fixed `src/cli.ts` formatting for `fish-lsp info`
- added `fish-lsp complete` completions for `-v/--version`, `-h/--help`,
`--help-*`, when 0 subcommands and/or flags are given

* docs: fixed `README.md` section order

* fix: `yarn pack` and `package.json` scripts appear to be working

- `yarn pack` creates a tarball of the package
- `package.json` scripts are updated to properly build the tarball
- added `files` field to `package.json` to include the directories
  necessary for the package to work *(main features working, but
  more testing is needed)*. See [package.json](./package.json) line 25
  for more details.

---

**What is this commit fixing?**

when a user tries to install the fish-lsp npm package, through their
package manager using something like:

```bash
npm install -g fish-lsp
pnpm install -g fish-lsp
yarn global add fish-lsp
```

The package manager previously needs to use typescript to build the
`src/**` directory, which is not necessary. This branch is testing
various methods of improving the package manager installation
experience.

---

**TODO/Notes**

- if we can get the wasm file to be included in the published
  package, then we can remove most of the `postinstall` related
  workarounds in the `package.json` scripts, which would be nice.

- previous attempts at publishing the `tree-sitter-fish.wasm` file
  have been made. Most noteably, in versions `v1.0.0`, `v1.0.1`, etc...
  the `wasm` file included did not properly bundle a wasm file with
  a correct [magic-number](https://www.google.com/search?client=firefox-b-1-d&q=wasm+magic+number)

- once the `postinstall`/`wasm` issues are resolved, building from
  source will no longer be necessary to install the package.

* fix: conf.d not included in background analysis

- removed yarn.lock
- added files to package.json
- removed .yarnrc.yml
- successfully built on npm, yarn, & pnpm by `pack` testings
- fixed minor misnumbered error code `src/diagnostics/errorCodes.ts`
- removed packageManager key from package.json because of corepack

* fix: rm *.wasm from .gitignore, added wasm to repo

- added yarn.lock back to .gitignore

* fix: added startup time w/ logger + popup support to `src/analyzer.ts`

- changes made to `package.json` scripts
  - `dev` script for local development
  - `dev:watch` script for local development
- `tsconfig.json` now contains a key for building `.tsbuildinfo` file
and `clean` package.json removes it
- `README.md` comments removed + fixes to other minor typo changes

* fix: deps in package.json `typescript version too hight for eslint`

- moved marked-man to `devDependencies`
- typescript now using 5.5.4 in `devDependencies`

* fix: ci.yml and added .npmignore

* fix: added tests to ci.yml and yarn.lock

* fix: fast-glob moved to dependencies

* docs: minor comment to README.md

* feat: fixed `src/execute-handler.ts` for testing

- issue visible in [pr #61](https://github.com/ndonfris/fish-lsp/pull/61/files)

Co-authored-by: branchvincent <[email protected]>

* docs: fix `README.md` Client Configurations (REQUIRED)

* chore: added @branchvincent to `.all-contributorsrc`

Signed-off-by: nick donfris <[email protected]>

* fix: `npm publish` should now work, `docs/MAN_FILE.md` updates, complete changes

- `fish-lsp complete` now outputs completions for `fish-lsp env
--no-global --no-local --no-comments --no-export`
- `fish-lsp info --time-startup` is now a valid operation
- man page was updated
- updated .npmignore & .gitignore
- updated README.md
- added functions to `src/cli.ts` for `--time-startup` operation
- added arguments to functions in `src/config.ts` for `fish-lsp env`
behavior
- pkg behaviour is still not working correctly

* fix: removed tests from `.github/workflows/ci.yaml` because of race condition

* docs: updated `docs/CHANGLOG.md`, `docs/CONTRIBUTING.md` and `README.md`

* docs: updated `docs/CHANGELOG.md` & `package.json` for v1.0.8-3

---------

Signed-off-by: nick donfris <[email protected]>
Co-authored-by: branchvincent <[email protected]>
@ndonfris ndonfris force-pushed the master branch 2 times, most recently from 7ba165f to 805784b Compare May 21, 2025 07:46
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.

Why is yarn required for the build?

2 participants