Use NAN_MODULE_WORKER_ENABLED to make module context aware #4
Conversation
All native modules used Electron be context-ware before when upgrading to Electron 14. [ref](electron/electron#18397). `superstring` is still one of the module that isn't context-aware. This commit uses NAN_MODULE_WORKER_ENABLED instead of `NODE_MODULE` to make the module context-aware. Please check electron/electron#18397 and nodejs/nan#792 for more detail.
|
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? |
|
Sorry folks, Superstring is one of the libraries where we do need more work to be usable. Only activating 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. |
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 |
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" |
|
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 bongnv/atom@94f2e4a. 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. |
|
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... |
|
What is the best way to test this in Pulsar with Electron 19? Do you edit the |
All native modules used in Electron need to be context-ware before when upgrading to Electron 14. ref. However,
superstringis still one of the module that isn't context-aware.This PR uses
NAN_MODULE_WORKER_ENABLEDinstead ofNODE_MODULEto make the module context-aware.Please check electron/electron#18397 and nodejs/nan#792 for more detail.
Testing was done locally.