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 compilation error with the latest @types/node #163

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Dec 31, 2019

Fixes electron/electron#21612

In the latest @types/node, NodeJS.EventEmitter was changed from class to interface. But some classes in electron.d.ts extend it (class cannot extend interface) so it causes a compilation error as I reported in above issue. This issue is not immediate because Electron's Node.js version is still v12. But fixing this issue is also valid for @types/node@12.

This patch fixes the issue with advice from @andrewbranch.

I confirmed above issue was resolved by electron.d.ts generated by this pull request branch (I had generated electron-api.json in the latest branch of electron repository).

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

FYI: The diff added to electron.d.ts by this pull request is as follows:

--- electron.old.d.ts	2020-01-01 00:46:19.000000000 +0900
+++ electron.d.ts	2020-01-01 00:47:29.000000000 +0900
@@ -8,6 +8,8 @@
 type GlobalEvent = Event;
 
 declare namespace Electron {
+  const EventEmitter: typeof import('events').EventEmitter;
+
   class Accelerator extends String {
 
   }
@@ -1645,7 +1647,7 @@
     webContents: WebContents;
   }
 
-  class BrowserWindow extends NodeJS.EventEmitter {
+  class BrowserWindow extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/browser-window
 
@@ -3025,7 +3027,7 @@
     state: string;
   }
 
-  class ClientRequest extends NodeJS.EventEmitter {
+  class ClientRequest extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/client-request
 
@@ -3465,7 +3467,7 @@
     value: string;
   }
 
-  class Cookies extends NodeJS.EventEmitter {
+  class Cookies extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/cookies
 
@@ -3622,7 +3624,7 @@
     scheme: string;
   }
 
-  class Debugger extends NodeJS.EventEmitter {
+  class Debugger extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/debugger
 
@@ -4151,7 +4153,7 @@
     show(): Promise<void>;
   }
 
-  class DownloadItem extends NodeJS.EventEmitter {
+  class DownloadItem extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/download-item
 
@@ -4530,7 +4532,7 @@
     restoreCompletedTransactions(): void;
   }
 
-  class IncomingMessage extends NodeJS.EventEmitter {
+  class IncomingMessage extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/incoming-message
 
@@ -5391,7 +5393,7 @@
     readonly currentlyLoggingPath: string;
   }
 
-  class Notification extends NodeJS.EventEmitter {
+  class Notification extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/notification
 
@@ -6258,7 +6260,7 @@
     label?: string;
   }
 
-  class Session extends NodeJS.EventEmitter {
+  class Session extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/session
 
@@ -7250,7 +7252,7 @@
     label: string;
   }
 
-  class TouchBarColorPicker extends NodeJS.EventEmitter {
+  class TouchBarColorPicker extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-color-picker
 
@@ -7262,7 +7264,7 @@
     selectedColor: string;
   }
 
-  class TouchBarGroup extends NodeJS.EventEmitter {
+  class TouchBarGroup extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-group
 
@@ -7272,7 +7274,7 @@
     constructor(options: TouchBarGroupConstructorOptions);
   }
 
-  class TouchBarLabel extends NodeJS.EventEmitter {
+  class TouchBarLabel extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-label
 
@@ -7285,7 +7287,7 @@
     textColor: string;
   }
 
-  class TouchBarPopover extends NodeJS.EventEmitter {
+  class TouchBarPopover extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-popover
 
@@ -7297,7 +7299,7 @@
     label: string;
   }
 
-  class TouchBarScrubber extends NodeJS.EventEmitter {
+  class TouchBarScrubber extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-scrubber
 
@@ -7313,7 +7315,7 @@
     showArrowButtons: boolean;
   }
 
-  class TouchBarSegmentedControl extends NodeJS.EventEmitter {
+  class TouchBarSegmentedControl extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-segmented-control
 
@@ -7326,7 +7328,7 @@
     selectedIndex: number;
   }
 
-  class TouchBarSlider extends NodeJS.EventEmitter {
+  class TouchBarSlider extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-slider
 
@@ -7340,7 +7342,7 @@
     value: number;
   }
 
-  class TouchBarSpacer extends NodeJS.EventEmitter {
+  class TouchBarSpacer extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/touch-bar-spacer
 
@@ -7461,7 +7463,7 @@
     transactionState: ('purchasing' | 'purchased' | 'failed' | 'restored' | 'deferred');
   }
 
-  class Tray extends NodeJS.EventEmitter {
+  class Tray extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/tray
 
@@ -7911,7 +7913,7 @@
     type: string;
   }
 
-  class WebContents extends NodeJS.EventEmitter {
+  class WebContents extends EventEmitter {
 
     // Docs: http://electronjs.org/docs/api/web-contents
 

@andrewbranch
Copy link

Two notes for reviewers to consider:

  1. This implies the existence of a value Electron.EventEmitter
  2. This would make electron.d.ts unusable by TypeScript 2.8 and below

/cc @sandersn

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

Thank you for the points. Electron._EventEmitter might be better to avoid confusion.

@sandersn
Copy link
Contributor

I created a 2.8-compatible PR that uses import events = require('events') and a declare global { wrapper. The bulk of the diff is similar, however. I'll create the PR shortly and the electron maintainers can decide if 2.8- compatibility is important.

@sandersn
Copy link
Contributor

Personally, I much prefer the diff from this PR. When we looked at general usage of Typescript from Visual Studio and VS Code a couple of months ago, 97% of VS users were on 3.0+ and 99.9% of VS Code users were. But of course there may be specific users of electron that are stuck on 2.8.

base/base_inner.ts Outdated Show resolved Hide resolved
src/module-declaration.ts Outdated Show resolved Hide resolved
@rhysd
Copy link
Contributor Author

rhysd commented Jan 5, 2020

@MarshallOfSound Thank you for the review. I addressed the points at a372a33.

@IceSentry
Copy link

Is there something else needed before merging this?

@MarshallOfSound MarshallOfSound merged commit 1872d1c into electron:master Jan 30, 2020
@electron-bot
Copy link

🎉 This PR is included in version 8.6.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

electron.d.ts does not work with @types/node v13.1.0
6 participants