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

Make this package context aware so that it can be used with the latest Electron versions #23

Closed
mariano-filipe opened this issue Jun 11, 2021 · 6 comments

Comments

@mariano-filipe
Copy link

I'm currently using this package with Electron 8 and node 10 and it's been working great. In trying to upgrade to Electron 13 and node 14, but I found an issue in my electron preload script:

Electron: Loading non-context-aware native module in renderer: '\\?\C:\Users\<redacted>\node_modules\@thiagoelg\node-printer\lib\node_printer.node'.
This is deprecated, see https://github.com/electron/electron/issues/18397.

While reading issue 18397 that Electron presents, I found out that this package contains a native module that is not context-aware which is something required in the latest Electron versions. I was wondering if any effort has already been done in this project to make it context-aware or even rewritten used this NAPI.

I'm not experienced in writing native modules, but the migration suggested in the issue seemed simple enough to me. I was willing to send a PR but first I had to ask if any effort has already been done in this direction. Any advice or workarounds are appreciated too.

@thiagoelg
Copy link
Owner

Hi, yeah.. this hasn't been updated to be ContextAware, but from reading the issue you tagged, it seems pretty straight forward (this is a great example of how to do it: atom/node-keytar/pull/182).

I don't think anyone attempted to update this module to be ContextAware, so feel free to open a PR.

As you can see, this package has next to 0 tests, so adding some tests to validate that all functions remain declared and working after the update is appreciated, and I think I can help a little with that.
Also, it's important that the library is built to work on Windows and POSIX (Linux/Mac), for that Travis and Circle should provide enough info if anything goes wrong.

Let me know if you need anything more specific and thanks for bringing this up!

@TimoKunze
Copy link

@mariano-filipe Have you tried refactoring your code to use this package from main instead? I'm using this package in Electron 13 without any problem, but I'm printing from main instead of renderer.

@nuwanwimalasena
Copy link

Im always getting "argument 1 must be an string" error. Im using node 14, but I can run examples without any issue. using same node version.

@thiagoelg
Copy link
Owner

Hey, @nuwanwimalasena, this seems like a different matter. Would mind opening a new issue with more details about the problem you're facing?

steinmetz85 added a commit to steinmetz85/node-printer that referenced this issue Sep 7, 2021
As mentioned in thiagoelg#23 the modul has to be context aware to be used in electron 14+.
I have tested the changes successfully for windows 10
thiagoelg pushed a commit that referenced this issue Oct 2, 2021
As mentioned in #23 the modul has to be context aware to be used in electron 14+.
I have tested the changes successfully for windows 10
@mariano-filipe
Copy link
Author

Hey @thiagoelg. Thanks a lot for merging the MR that brings context awareness to this package. Could you please launch a release for this package containing this change? It would help us a lot.

@thiagoelg
Copy link
Owner

Hi @mariano-filipe , oops, forgot to do it when I merged the MR. Should be published now as v0.5.6 😄

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

No branches or pull requests

4 participants