megahertz / electron-log

Simple logging module Electron/Node.js/NW.js application. No dependencies. No complicated configuration.
MIT License
1.3k stars 127 forks source link

fix: ElectronExternalApi fallback to super when electron app is undefined, fixes #399 #400

Closed douglascayers closed 8 months ago

douglascayers commented 8 months ago

Background

Fixes #399

Changes

megahertz commented 8 months ago

Thank you, @douglascayers. The PR looks good! I would only prefer to pass electron as an argument of the ElectronExternalApi constructor instead of mocking in tests. I'm able to make that change this weekend, or you can make that change by yourself.

Something like that:

const electron = require('electron'):
...
const externalApi = new ElectronExternalApi({ electron });
douglascayers commented 8 months ago

I would only prefer to pass electron as an argument of the ElectronExternalApi constructor instead of mocking in tests

Good with me! That would make the test much easier to stub the electron props and avoid need for testdouble.

I'm able to make that change this weekend, or you can make that change by yourself.

Thanks, I'll adjust the PR.

douglascayers commented 8 months ago

@megahertz I've refactored the changes -- much easier to test using dependency injection 😄

Ready for your review again, thanks

megahertz commented 8 months ago

Thank you @douglascayers. It looks great! I will try to merge it tomorrow morning.