search

seriema / electron-notification-shim

Get Notification API events in Electron main-process. Perfect for adding Notification toasters in Windows with node-notifier or other solution.
MIT License
37 stars 5 forks source link

Fix deprecated dependency ipc #1

Closed beshur closed 8 years ago

beshur commented 8 years ago

require('ipc') is deprecated

seriema commented 8 years ago

Thanks @beshur ! Just wanted to update you that I saw this but I haven't been able to test it out yet. Want to make sure everything keeps working, and with Easter coming up the time just flies. I hope to test this ASAP!

seriema commented 8 years ago

From what I found, the deprecation happened in Electron 0.35.0, but it's not removed yet? Since electron-notification-shim depends on "electron-prebuilt": "^0.34.3" it's not actually expecting >=0.35.0 (npm docs).

@beshur we should probably update package.json to match what the code expects, so the version dependency range should be bumped to ^0.35.0. Would you like to update the PR? This doesn't really affect projects depending on electron-notification-shim, so I'm not sure how to make this noticeable and any tips are welcome.

beshur commented 8 years ago

Yes, I can edit the PR, but wanted to notice that this module will be used with some other project with their own electron-prebuilt version, which will override module's dep. Also the guys are rolling out electron releases really quick, so it will be obsolete in some time I guess.

seriema commented 8 years ago

Yeah and that makes things tricky as I don't want to flood other projects console with "deprecated" or "ipcRenderer is undefined" messages depending on what electron version they use.

Something like npm engines would be very useful, and Sindre's electron-boilerplate uses an electronVersion that probably would solve this but I can't find any official docs on it.

Maybe a simpler approach for now might be to simply detect the presence of ipcRenderer and use it if it's there, otherwise fall back to the old way. What do you think about that @beshur ? Then the devDependencies in package.json doesn't need to be changed.

beshur commented 8 years ago

@seriema, sorry, didn't have time for this.

I wanted to test my fix on older versions to prove it is safe, but didn't manage to.

seriema commented 8 years ago

@beshur no problem! I finally got around to doing it and found a way to make it work on older versions (by doing a require('electron') within a try/catch block and calling require('ipc') when it fails).

beshur commented 8 years ago

Well, I would better try something more sophisticated, so that catch is not a standard behavior.

seriema commented 8 years ago

I'm not a fan of that either, but from what I found there isn't a simple standard way of checking for what version of Electron it's running in. Or even if a npm module is available. fs could be an option but it feels very error prone, especially given npm 2 and 3 having different file structures. This SO question was what convinced me: https://stackoverflow.com/questions/21740309/how-to-check-in-node-if-module-exists-and-if-exists-to-load

I'd gladly accept a PR if you have a cleaner solution.

beshur commented 8 years ago

I see. Will ping you if I come up with anything.

seriema commented 8 years ago

@beshur glad we did this, since Electron 1.0.0 was released a week after! haha And their first migration notice instruction is: "Please update your app to v0.37.8 first to check whether there are deprecated APIs usages."

1.1.0 is already out too. So we can probably drop the deprecation hack check soon. ^^ Although I might reuse the way it's done when they deprecate something else, unless a better way presents itself.