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

Making it compile and run for node 12+ and electron 6+ #261

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

thiagoelg
Copy link
Contributor

@thiagoelg thiagoelg commented Sep 17, 2019

This was my first time coding for a node and, specially, coding in c++ (for v8), so any feedback is appreciated.

These changes are mostly me banging my head to make this compile and run for the latest node and electron versions without breaking older ones.

As you can see the macros.hh file is kind of messy, as I was modifying it as things started to work or break.

With these changes this lib is able to compile from node v0.12 up to node v12, along with electron 1.2.8 up to 6.0.7 (and probably later), though I was not able to test for every version.

Compilations via node versions pre v6 are not possible, but node-pre-gyp is able to compile them using the --target flag and getting the correct headers.

I tested for electron 1.7.2 and 6.0.7 and getPrinters and printDirect are working correctly.

Even if this is not merged, hopefully it'll help others updating node-printer to support more recent node releases.

I published this under @thiagoelg/node-printer on npm.

@ekoeryanto
Copy link
Contributor

ekoeryanto commented Sep 30, 2019

I released it with prebuild for my own usage https://github.com/ekoeryanto/node-printer.

thanks @thiagoelg

EDIT:
available on npm with @pake/node-printer name

@iamtomcat
Copy link

@ekoeryanto I couldn't get the pre-built libraries for electron 6 to run. Kept saying it was build for node version 48. Also I'm on macos.

@ekoeryanto
Copy link
Contributor

@iamtomcat I tried electron 6 build with electron builder yesterday, it works.

@iamtomcat
Copy link

@ekoeryanto Cool, yeah I'm new to electron so just figuring things out. Is there anything you have to set to make sure it grabs the correct version. I've done the electron-rebuild and that's what seems to be getting the wrong package.

@ekoeryanto
Copy link
Contributor

ekoeryanto commented Oct 16, 2019

@iamtomcat, can you create an issue in the forked repo, please?

@ekoeryanto
Copy link
Contributor

Hi @thiagoelg
I tried to compile it with electron 7 but it throws this error

/tmp/prebuild/electron/7.0.0/include/node/v8.h:3458:37: note:   candidate expects 3 arguments, 2 provided
../src/node_printer_posix.cc: In function ‘void getSupportedPrintFormats(const Nan::FunctionCallbackInfo<v8::Value>&)’:
../src/node_printer_posix.cc:446:69: error: no matching function for call to ‘v8::Array::Set(int, v8::Local<v8::String>)’
         result->Set(i++, V8_STRING_NEW_UTF8(itFormat->first.c_str()));

@thiagoelg
Copy link
Contributor Author

Hmm... I never got to test with Electron 7, there must be some new V8 API changes. I might take a look next weekend.
The current implementation of the macros used are way too sensitive to API changes :/

@thiagoelg
Copy link
Contributor Author

Hey @ekoeryanto, I made it compile for Electron 7, not sure if still works, didn't have time to test, but you can check it here:
thiagoelg#7

Also, I borrowed your changes to make it possible to use prebuild. Thanks!

@ekoeryanto
Copy link
Contributor

Hi, @thiagoelg I'm happy you published your, I am going to archive mine and use yours.

@aalvarez255
Copy link

Hi @thiagoelg, I can't make your repo work with electron 7. I'm missing v75 node version (node-printer-v0.5.2-electron-v75-win32-x64.tar.gz).

@ekoeryanto did you manage to get it working with electron 7?

Thanks.

@thiagoelg
Copy link
Contributor Author

@aalvarez255 Yeah, appveyor didn't build for electron 7 for some reason. I built it manually and uploaded it to the 0.5.2 release.
Can you try again?

@aalvarez255
Copy link

@thiagoelg it's working fine now. Thanks :)

@ghost ghost mentioned this pull request Nov 20, 2019
Copy link
Owner

@tojocky tojocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@tojocky tojocky merged commit ad87c74 into tojocky:master Nov 26, 2019
@aalvarez255
Copy link

@tojocky could you publish updated master branch to npm please? thanks :)

@thiagoelg
Copy link
Contributor Author

I've made some new changes (based on the work from @ekoeryanto) on my fork that makes publishing prebuilt packages for all node/electron versions easier. I can make a new PR here with those changes if @tojocky agrees :)

@tojocky
Copy link
Owner

tojocky commented Dec 11, 2019

@tojocky could you publish updated master branch to npm please? thanks :)

Please check v0.4.0 can work with node v13 as well.

@tojocky
Copy link
Owner

tojocky commented Dec 11, 2019

I've made some new changes (based on the work from @ekoeryanto) on my fork that makes publishing prebuilt packages for all node/electron versions easier. I can make a new PR here with those changes if @tojocky agrees :)

please do

@thiagoelg thiagoelg deleted the to-src branch August 30, 2021 04:31
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

Successfully merging this pull request may close these issues.

5 participants