Make the module Context Aware

[Lan-Hekary]

Hello, this is a solve for the issue #102

I tested this on Electron 8.3.4 with app.allowRendererProcessReuse = true; and app.allowRendererProcessReuse = false;

in both tests the module behaves as expected.

I don't know if there is further modification to be done (Cleanup or some other Tweaks).

this PR is Inspired by this PR and this Commit

I hope it helps :D

[Lan-Hekary]

after further testing .. I am using electron-reload and if I reload the app with app.allowRendererProcessReuse = true; , the registered Callbacks does not work anymore.

I am no Expert at node so I need pointers at how to fix this problem.

[MadLittleMods]

I am using electron-reload and if I reload the app with app.allowRendererProcessReuse = true; , the registered Callbacks does not work anymore.

Did this work previously?

---

Thanks for the references for what you based this on :+1:

[Lan-Hekary]

Did this work previously? it Works only if app.allowRendererProcessReuse = false; I can reload the app at any time and it will register the callbacks at every startup normally.

I am using Electron 8 so "allowRendererProcessReuse" is turned off by default but in Electron 9 it will be enabled by default plus I am getting Warnings that I am "Loading non-context-aware native module in renderer".

anyway when I turn it on with the current module, the whole app crash off course because the module is non-context-aware.

but with my Pull Request the App Continue to Work Normally Unless you actually have to Reuse the Renderer Process.

I guess the problem is that I have to detect when the Context has changed and unregister the old callbacks. and this has to be done in the module itself, but I have no Idea how to make it work.

[timfish]

App Continue to Work Normally Unless you actually have to Reuse the Renderer Process

The old callbacks will not work after the renderer has been refreshed because the previous browser context no longer exists. Or are you saying that in the new context, callbacks are not working?

The changes so far in this PR allow the module to be used in recent Electron but it'll probably be leaking memory everywhere. We'll almost certainly need to implement the cleanup hook to ensure that resources are freed. Here is an example of the required hook.

My c++ skills are not up to scratch so I'm not 100% sure what needs to be changed. It looks like at a minimum Stop() should be called in the cleanup hook and any usages of static need to be STATIC_THREAD_LOCAL. These are these are just things I've seen in PRs to fix this issue in other modules 🤷‍♂️.

Some modules have decided that this is a good time to migrate to N-API which doesn't have these issues.

[MadLittleMods]

@timfish Thanks for the extra details on what we need to do! ❤️

Is this PR good as a small iteration? Does it break any use cases that worked before? If it fixes some use case and not our existing ones, I'm ok to merge and create an issue to track the debt (known issue).

Here is the issue we have for switching to N-API but I haven't looked into this at all: https://github.com/MadLittleMods/node-usb-detection/issues/42

[Lan-Hekary]

as far as I know this PR is not breaking existing compatibility but it will need more work and testing ( I will try to work on testing in my free time but it's a slow pace)

as for N-API .. I honestly have no Idea How to migrate. it will need some research from me. in the mean time feel free to merge the current work but put a Clear Warning that this Module is not Fully Context Aware yet and people should do their best to report issues and I will try my best to keep track with you.