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

Migrate to n-api #5

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Migrate to n-api #5

wants to merge 3 commits into from

Conversation

bongnv
Copy link

@bongnv bongnv commented Nov 22, 2022

This PR migrates superstring to n-api. It passed all unit tests with a few caveats:

  • v8::Set, v8::RegExp and node::ErrnoException aren't available in n-api so I used JsValueFromV8LocalValue to convert v8::Value to napi_value after using v8 and node API directly.
  • this native addon is still not thread-safe because it still still uses static variables like static Napi::FunctionReference constructor
  • will not work with the current tree-sitter because tree-sitter uses nan-API of superstring (ref)
  • noop delete was removed because it's 'noop'

@mauricioszabo
Copy link

So, while this took a lot longer to respond, maybe there is still some usage for this migration.

Originally, I found that it was more useful to migrate everything to the WASM version of Superstring. Unfortunately, on very big files (8k to 15k) the editor is struggling and typing lag is clearly visible and it interferes with the workflow.

So I'll be testing these changes on the PR to see if they work on newer Electron, and how much impact they do.

@mauricioszabo
Copy link

Quick update - it works, but not 100%.

It crashes with the error:

Cannot set property score of #<SubsequenceMatch> which has only a getter", source: /home/mauricio/projects/pulsar_repos/pulsar/node_modules/autocomplete-plus/lib/subsequence-provider.js

Basically, the Subsequence matcher is defined here on Superstring, but I remember it was some of the things that crashed the editor

@mauricioszabo mauricioszabo mentioned this pull request Oct 18, 2023
@mauricioszabo
Copy link

@bongnv, after A LOT of testing, I have to say - GREAT JOB!

The Subsequence matcher bug happens some times (but not always - not sure why, honestly, but I feel I can work on a fix) and this fork is being used on the newer Electron. As soon as I work on a fix, I feel we can merge this!

I also saw some bugs on VIM Mode that, supposedly, start from Superstring but I need to reproduce them and be sure they are not also happening on the "stable" version of Superstring too.

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.

2 participants