mikaelbr / node-notifier

A Node.js module for sending notifications on native Mac, Windows and Linux (or Growl as fallback)
MIT License
5.73k stars 322 forks source link

Can't load module if in .asar #76

Closed kurisubrooks closed 8 years ago

kurisubrooks commented 8 years ago

When the module is compiled inside an Electron Binary File, the module will not load.

I've tried everything I can think of, but it refuses to load. Here's my current syntax that i'm using to require the module:

require(path.join(__dirname, '../lib', 'node-notifier'));

Everything else seems to work (using similar syntax), but node-notifier refuses to load.

This is the path's output. (Looks fine)

/Users/Kurisu/Applications/eew.app/Contents/Resources/app.asar/lib/node-notifier
lipis commented 8 years ago

I like the urgent.. :)

kurisubrooks commented 8 years ago

Well it's kind of a critical bug. People want to use node-notifier in their apps, but when compiling it to an app, if it doesn't work, i'd say that's quite critical and needs to be urgently fixed.

kurisubrooks commented 8 years ago

I've looked through the repo code, but I can't see any reason for it not to be working, which is a real annoyance as I hoped it would have been something obvious that could be fixed with a couple lines of code, but it doesn't seem that way.

As for me, i'm still learning Node so it might be obvious to @mikaelbr, but, who knows. I hope this gets patched soon though.

lipis commented 8 years ago

We can only hope so.. and yes.. I agree that is quite critical!

mikaelbr commented 8 years ago

I think this has something to do with the bundling of electron. I need to get familiar with how that works. But if I were to guess, it would be for that electron have issues with bundling *.app files.

kurisubrooks commented 8 years ago

The problem is that Electron compiles the included app files into a bundled file called app.asar. .asar's are compressed almost identically to a .tar archive, but it's still normally accessible like a file system. For some reason, out of my 20 different modules that I use in my program, node-notifier is the only one that fails to work in it.

mikaelbr commented 8 years ago

That is probably due to it using binaries as dependencies (the bundled terminal-notifier.app/toaster.exe and notifu.exe).

kurisubrooks commented 8 years ago

Ah that might be so. I hope you'll be able to fix this soon :disappointed:

mikaelbr commented 8 years ago

For future reference (for my self as well). Probably related: https://github.com/atom/electron/blob/master/docs/tutorial/application-packaging.md#extra-unpacking-on-some-apis

mikaelbr commented 8 years ago

I've looked some into this, and you should now be able to pack into asar package (with the current master). Though, you have to add vendor files as unpacked files to the asar archive. Unless you do this, you will get access error when doing notification (unable to access the execution files - couldn't find a way around this.)

Example packing that worked fine for me:

asar pack . node-notifier.asar --unpack "./vendor/**"

Would be great if you could try this out in master and let me know how it goes.

kurisubrooks commented 8 years ago

I'll have to try this out later, thanks! I'll be busy for the next 2 days, but I'll be sure to test asap. If all goes well, do you want me to add this to the README, or will you prefer to do that?

mikaelbr commented 8 years ago

You can do it if you want. I'll do it if not.

kurisubrooks commented 8 years ago

I'm trying to test, but asar is giving me some troubles. Will let you know soon how it turns out.

bennycode commented 8 years ago

Good to see that you are working on it! I'll keep my fingers crossed!

lipis commented 8 years ago

Any news here guys?

kurisubrooks commented 8 years ago

I haven't been able to get it to work as of yet, but I think this is due to a problem with electron-packager. I'm still yet to get it working :disappointed:

mikaelbr commented 8 years ago

It seems to work for me? By using the command:

asar pack . node-notifier.asar --unpack "./vendor/**"

This is to make an asar for node-notifier. If you are to build asar-packages for something dependent on node-notifier you would have to re-specify the path:

asar pack . some-package.asar --unpack "./node_modules/node-notifier/vendor/**"
kurisubrooks commented 8 years ago

It works! I had to do some hacky work arounds to get my Electron wrapper to work with it, but packaging the app into the asar with the provided command does indeed fix this issue.

I'll go ahead and add it to the README, then close this issue.

lipis commented 8 years ago

I still didn't manage to make it work.. I'm a bit new to both project so bare with me :)

I unpacked the node notifier but then what?! How do I use it?

Here I created this minimal project that shows one notification on click. Could anyone show me how/what should I update in order to be able to package it using the electron-packager?

https://github.com/lipis/electron-experiments

kurisubrooks commented 8 years ago

First npm i electron-packager -g, then go into the directory you want to package. Run the following command to generate the binaries:

electron-packager ./ test --platform=all --arch=all

Once you have the binaries, run:

asar pack . app.asar --unpack "./node_modules/node-notifier/vendor/**"

(change the module directory if needed)

Lastly, open the resources directory (changes depending on OS), delete the app folder, then copy the output app.asar and app.asar.unpacked to this folder.

Run the electron binary and give it a go. @lipis

lipis commented 8 years ago

@kurisubrooks It worked!! Thanks a lot to both of you guys :) :dancer: :dancers: :dancer:

kurisubrooks commented 8 years ago

No prob! Glad we got it working for you :smile:

bennycode commented 8 years ago

:dancers:

sean6bucks commented 8 years ago

Still having problems with this on Win7 balloon notifications. Is this fix (asar unpacking) only for the OSX and Win8 notifications? Anyone have any luck using this for balloons?

Unfortunately I can't get debugging working in my main process so I can't even see if its throwing errors.

kurisubrooks commented 8 years ago

I don't see why it wouldn't work for Balloon notifications. Have you tried the command exactly as we mentioned above/in the README? @sean6bucks

sean6bucks commented 8 years ago

Yeah, did it exactly as above and was able to get the app.asar file as well as app.asar.unpacked folder whcih is structured as so (Im only using Toaster and Notifu):

resources/app.asar.unpacked/node_modules/node-notifier/vendor/ which contains

notifu/

toaster/

I also did an asar list of the app.asar and it lists those files still in the asar as well, so I tried packing it without them and keeping the unpacked folder but that didn't work either.

(Sorry, total fail on teh structure diagram, improving)

kurisubrooks commented 8 years ago

What files are in app.asar.unpacked? This is the directory that the unpacked files should be in. They should not be moved out of this folder. @sean6bucks

sean6bucks commented 8 years ago

updated, I have the Notifu exe's and toast exe & dll's in unpacked

kurisubrooks commented 8 years ago

It should look something like this:

./
└─ app.asar/
└─ app.asar.unpacked/
    └─ node_modules/
        └─ node-notifier/
            └─ vendor/
                └─  notifu/**
                └─  toaster/**
                └─  terminal-notifier/**

Does this not work for you? @sean6bucks

sean6bucks commented 8 years ago

yup, thats exactly what I have minus the terminal notifier folder since Im not using it

kurisubrooks commented 8 years ago

Would it be possible to find some sort of logging module that outputs the console log to a file, so we can see what's going on? I can't help without some sort of error or warning... I need to see something as to why this might be happening... Sorry

lipis commented 8 years ago

@kurisubrooks Now I have another problem and that's with the icon. Lets say that my icon is in the img folder so I'm using the following path in my Electron: path.join(app.getAppPath(), 'img', 'icon.png'). Works as expected in dev environment but when I pack it.. it's not :)

So my question is.. how could I pack by ignoring two folders now.. including the img as well :)

lipis commented 8 years ago

On production it tries to read the image from this path:

C:\Users\IEUser\AppData\Local\foo\app-2.0.2457\resources\app.asar\img\icon.png

and obviously fail. I though about have the icons unpacked as well and then simply replace the app.asar with app.asar.unpacked but I didn't managed to unpack two paths. Something like this (broken):

--unpack "./node_modules/node-notifier/vendor/** | ./img/**"
kurisubrooks commented 8 years ago

Firstly, Instead of using app.getAppPath(), consider using path.join(__dirname, 'img', 'icon.png');. This has worked for me.

As for your question, i'm unsure of how we'd be able to do something like --unpack "./path/to && ./path/to"? I haven't tested this.

Let me know if either of these work for you @lipis

lipis commented 8 years ago

We tried a lot of combinations of &&, |, ,, etc.. but nothing seems to work. As a workaround we might put the images inside the vendor for now until this could be fixed somehow..

kurisubrooks commented 8 years ago

Darn that's disappointing. Maybe go make an issue on their repo asking for a feature request.

That's what I currently do with my app. I have a folder that I have all the exports in, then just export that folder.

lipis commented 8 years ago

https://github.com/atom/asar/issues/45 Let's see

mikaelbr commented 8 years ago

The check in asar happens here: https://github.com/atom/asar/blob/master/src/asar.coffee#L39

It uses minimatch and globs to match, so you can test your patterns by doing something like:

var minimatch = require('minimatch');

var pattern = '{./node_modules/node-notifier/vendor/**,./img/**}';
console.log(minimatch('./img/icon.png', pattern, { matchBase: true }));
console.log(minimatch('./node_modules/node-notifier/vendor/terminal-notifier.png', pattern, { matchBase: true }));
sean6bucks commented 8 years ago

Hey @kurisubrooks just an update on the Win7 Balloon problems I was having. Finally got around to get a debug of the code and was receiving the error

"Error: ENOENT, node_modules/node-notifier/vendor/notifu/notifu not found in ..MyApp/resources/app.asar"

So I took a look in the balloon.js and notice it was looking for ".../notifu/notifu" which I assume is to handle the regular .exe and the 64.exe. For this we are only building a 32 bit version, so I changed the .js to notifu.exe and it fixed the problem.

Not sure if this will help fix the problem if you need x32 & x64, but just wanted to let you guys know. Thanks!

kurisubrooks commented 8 years ago

Thanks for the heads up! @mikaelbr might want to look into this. 

mikaelbr commented 8 years ago

Yeah, I can check the underlying architecture type and explicitly set whether or not to use 32 or 64 bit version manually.

lipis commented 8 years ago

In other news.. so it looks like electron will support notifications out of the box https://github.com/atom/electron/commit/765cfb10941dd3b04566b01ce71eb78e730fd742

kurisubrooks commented 8 years ago

I've already been aware of Electron's notification APIs for some time, but it looks like they've finally got Windows integration somewhat working. Can't wait to see how they're going to integrate this, and what kind of features it will have.

lipis commented 8 years ago

I'm using node-notifier only for Windows anyway.. so looking forward to that as well. No more asar hacks.. :)

Darmody commented 8 years ago

@kurisubrooks Hi, I met this problem too.And I pass the asar-unpack-dir options to electron-packager, I get Resources dir like this: image

But, I still can't get notification, and the error is :

image

Did I miss sth?

kurisubrooks commented 8 years ago

@Darmody ENOENT means the directory doesn't exist, so it can't find the module. Double check you unpack the right folder. If you're sure, it doesn't look like you're getting the proper location, you can try something like this:

const path = require('path');

// path to terminal-notifier:
var term_path = path.join('__dirname', 'node_modules', 'vendor', 'terminal-notifier.app', 'Contents', 'MacOS', 'terminal-notifier');
Darmody commented 8 years ago

@kurisubrooks Thanks for your help, I think it is location problem too, I'll try the code you mentioned.

mikaelbr commented 8 years ago

Note, it should probably be

var term_path = path.join(__dirname, 'node_modules', 'vendor', 'terminal-notifier.app', 'Contents', 'MacOS', 'terminal-notifier');

Not

var term_path = path.join('__dirname', 'node_modules', 'vendor', 'terminal-notifier.app', 'Contents', 'MacOS', 'terminal-notifier');

(Notice in the second example __dirname is a string, not a "global variable")