Use NAN_MODULE_WORKER_ENABLED to make module context aware

[bongnv]

All native modules used in Electron need to be context-ware before when upgrading to Electron 14. ref. However, superstring is still one of the module that isn't context-aware.

This PR uses NAN_MODULE_WORKER_ENABLED instead of NODE_MODULE to make the module context-aware.

Please check https://github.com/electron/electron/issues/18397 and https://github.com/nodejs/nan/pull/792 for more detail.

Testing was done locally.

[confused-Techie]

The tests are looking good here. and since he's been working on getting this context-aware @mauricioszabo have anything to add on this PR?

[mauricioszabo]

Sorry folks, Superstring is one of the libraries where we do need more work to be usable. Only activating NAN_MODULE_WORKED_ENABLED is not sufficient - it crashes the editor.

More info here: https://github.com/pulsar-edit/pulsar/issues/89

[confused-Techie]

Sorry folks, Superstring is one of the libraries where we do need more work to be usable. Only activating NAN_MODULE_WORKED_ENABLED is not sufficient - it crashes the editor.

More info here: pulsar-edit/pulsar#89

Thanks for the information. We can keep the PR open, since both of the other two here are also working on the same endgoal.

[bongnv]

Sorry folks, Superstring is one of the libraries where we do need more work to be usable. Only activating NAN_MODULE_WORKED_ENABLED is not sufficient - it crashes the editor.

More info here: pulsar-edit/pulsar#89

Does it crash because of 'memcpy'? I'm using this branch with Atom + Electron 13 and it seems fine.

[mauricioszabo]

Does it crash because of 'memcpy'? I'm using this branch with Atom + Electron 13 and it seems fine

@bongnv do you have a branch/repo that you can share so I can see what's wrong? I'm quite sure I saw crashes on both Electron 13, 15, and 19 by just enabling NAN_MODULE_WORKED_ENABLED.

Basically, it crashed with a default Pulsar installation with only the core plug-ins. Trying to open a text editor and typing 4 characters crashed the whole process

[mauricioszabo]

We can keep the PR open

Yes, we can, but it needs more work to be able to keep the module as being NAN.

So, we can keep this PR open, and if someone feels its easier to migrate to N-API, there's a work in progress here: pulsar/issues/30#issuecomment-1309834505

Finally, we can also work on the WASM version here: https://github.com/pulsar-edit/superstring-wasm, but there are three issues (and more to come) that we need to handle so that we can be considered "stable"

[bongnv]

I modified atom quite a lot and test those modules here https://github.com/bongnv/atom/commits/next. I upgraded to Electron 13 in here https://github.com/bongnv/atom/commit/94f2e4a1ec1a4ff5121b28c3b48e97b4549deff7. Electron 13 doesn't require context-aware addons so this module may crash on Electron 14 (which will require). I haven't tested on Electron 14 yet. I'll try after getting all modules context-aware.

Migrating to n-api would probably be better because NAN_MODULE_WORKED_ENABLED just ignores the context.

[mauricioszabo]

On my work to migrate to N-API, I saw a quite huge performance hit, just so you know. WASM I believe was even faster...

[fabianfiorotto]

What is the best way to test this in Pulsar with Electron 19? Do you edit the .cc files in node_modules and run yarn run build? I tried to test these changes in the branch Mauricio referenced and I get a lot of errors regarding tree-sitter I can't even tell if this is working or not.

[mauricioszabo]

Folks, I'm closing this because the migration to N-API, that's on this PR: https://github.com/pulsar-edit/superstring/pull/5 is actually working and everything is correct in newest Electron :)