search

thiagoelg / node-printer

Native node.js printer
125 stars 76 forks source link

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

Closed mariano-filipe closed 2 years ago

mariano-filipe commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

mariano-filipe commented 2 years ago

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 commented 2 years ago

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