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: make sure classes in lib correctly implement Electron interfaces #40291

Merged
merged 1 commit into from Oct 25, 2023

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Oct 20, 2023

We are already doing it in some places such as

export class ClientRequest extends Writable implements Electron.ClientRequest {

Let's do it consistently everywhere

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 20, 2023
@miniak miniak self-assigned this Oct 20, 2023
@miniak miniak added semver/patch backwards-compatible bug fixes no-backport labels Oct 20, 2023
@@ -15,7 +15,7 @@ class AutoUpdater extends EventEmitter {
}

getFeedURL () {
return this.updateURL;
return this.updateURL ?? '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the Mac implementation returns an empty string here if the URL is not specified. We should either do that here as well, or change the documentation to allow null

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 21, 2023
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

The change LGTM.

Looks like the linux build has failing tests, but I can't tell if those are related to this change or not. Re-running them.

@miniak
Copy link
Contributor Author

miniak commented Oct 25, 2023

The change LGTM.

Looks like the linux build has failing tests, but I can't tell if those are related to this change or not. Re-running them.

they are green now

@ckerr ckerr merged commit f66d4c7 into main Oct 25, 2023
14 checks passed
@ckerr ckerr deleted the miniak/class-implements branch October 25, 2023 18:02
@release-clerk
Copy link

release-clerk bot commented Oct 25, 2023

No Release Notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants