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

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

Merged
merged 3 commits into from
Feb 1, 2024
Merged

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

merged 3 commits into from
Feb 1, 2024

Conversation

douglascayers
Copy link
Contributor

@douglascayers douglascayers commented Jan 26, 2024

Background

Fixes #399

Changes

  • Update ElectronExternalApi.getAppName() and ElectronExternalApi.getAppVersion() to fallback to super method just as when catching an error if electron.app is undefined
  • Inject electron into api class to help with testing and mocking scenarios
  • Add tests for ElectronExternalApi class

@megahertz
Copy link
Owner

megahertz commented Jan 26, 2024

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
Copy link
Contributor Author

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 douglascayers marked this pull request as draft January 30, 2024 02:32
@douglascayers douglascayers marked this pull request as ready for review January 30, 2024 05:00
@douglascayers
Copy link
Contributor Author

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

Ready for your review again, thanks

@megahertz
Copy link
Owner

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

@megahertz megahertz merged commit a86e48f into megahertz:master Feb 1, 2024
7 checks passed
@douglascayers douglascayers deleted the fix/electron-external-api-fallback branch February 1, 2024 12:39
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.

Unhandled electron-log error TypeError: The "path" argument must be of type string. Received undefined
2 participants