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

electron.d.ts does not work with @types/node v13.1.0 #21612

Closed
3 tasks done
rhysd opened this issue Dec 24, 2019 · 81 comments · Fixed by electron/typescript-definitions#163
Closed
3 tasks done

electron.d.ts does not work with @types/node v13.1.0 #21612

rhysd opened this issue Dec 24, 2019 · 81 comments · Fixed by electron/typescript-definitions#163

Comments

@rhysd
Copy link
Contributor

rhysd commented Dec 24, 2019

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

  • Electron Version:
    • 7.1.7
  • Operating System:
    • macOS 10.14
  • Last Known Working Electron version:
    • N/A

Expected Behavior

Can compile successfully the TypeScript code

Actual Behavior

Compilation error

main/window.ts:14:5 - error TS2339: Property 'removeAllListeners' does not exist on type 'BrowserWindow'.

14 win.removeAllListeners();
       ~~~~~~~~~~~~~~~~~~

node_modules/electron/electron.d.ts:1655:31 - error TS2689: Cannot extend an interface 'NodeJS.EventEmitter'. Did you mean 'implements'?

1655   class BrowserWindow extends NodeJS.EventEmitter {
                                   ~~~~~~

EDIT: Added entire compile error messages thanks to #21612 (comment)

To Reproduce

Try to compile following code with the latest TypeScript compiler and latest @types/node package.

import { BrowserWindow } from 'electron';
const win = new BrowserWindow();
win.removeAllListeners();

Package versions:

  • typescript: 3.7.4
  • @types/node: 13.1.0

Screenshots

Nothing

Additional Information

I have investigated this issue and found the cause.

@types/node v13 has breaking change affecting NodeJS.EventEmitter from v12. It was changed from class to interface. Now, EventEmitter can only be extended by interfaces. I confirmed electron.d.ts worked fine with @types/node v12. The change is here:

DefinitelyTyped/DefinitelyTyped@c47a34e#diff-a2f9a5377787f7084c7f52b20c0108cfR540

However, classes such as BrowserWindow in electron.d.ts try to extend EventEmitter so it is causing this issue.

May be related to #21475

@rhysd
Copy link
Contributor Author

rhysd commented Dec 24, 2019

I believe we can no longer use NodeJS.EventEmitter and we need to use import { EventEmitter } from 'events'; for this. However, import statement is not available since <reference/> is used and we currently rely on Electron namespace. It probably requires rewriting to use import/export statements instead of <reference/>/namespace in electron.d.ts.

@basarat
Copy link

basarat commented Dec 24, 2019

The @types/node for node 13 also result in the following type error:

Cannot extend an interface 'NodeJS.EventEmitter'. Did you mean 'implements'?

More

DefinitelyTyped/DefinitelyTyped@c47a34e#r36559684

@rhysd
Copy link
Contributor Author

rhysd commented Dec 24, 2019

Yeah, the message is caused by the above compile error. And implements is (of course) not available for us. I included the error message in description of this issue. Thanks!

rhysd referenced this issue in DefinitelyTyped/DefinitelyTyped Dec 24, 2019
* feat(node): v13

* feat(node): v13.2
@amirburbea
Copy link

Running into same issue

@vladimiry
Copy link

vladimiry commented Dec 24, 2019

It's expected as Electron v7 comes with node v12, so use the most recent @types/node v12.

@amirburbea
Copy link

amirburbea commented Dec 24, 2019

No @vladimiry . That's not how it works - there is no direct tie between @types/node and the version of nodejs. Many packages (not just electron) reference @types/node using a version range (such as "^8.0.0" or worse "*"), so when one of them upgrades everyone really must be able to.

Electron incompatibility with the latest @types/node makes it hard to upgrade any package in my solution

@vladimiry
Copy link

there is no direct tie between @types/node and the version of nodejs

Can you provide a source?

@amirburbea
Copy link

Even if I can't (which I'm sure I could if I spent the time), the problem is most solutions reference many packages that reference @types/node via a version range.

The problem is electron types declare a class extends NodeJS.Emitter but that it's an interface now so the correct syntax is implements NodeJS.Emitter or more correctly extends events.EventEmitter which works with old node and new node

@rhysd
Copy link
Contributor Author

rhysd commented Dec 25, 2019

It's expected as Electron v7 comes with node v12, so use the most recent @types/node v12.

At least Electron v8 beta and v9 nightly have the same issue. Is there any plan to fix this incompatibility between electron.d.ts and @types/node v13 before upgrading node version of Electron? I concern the case where electron.d.ts is not fixed but Electron's node version is upgraded to v13.

@vladimiry
Copy link

At least Electron v8 beta and v9 nightly have the same issue.

Electron 8.0.0-beta.5 comes with Node 12.13.0, didn't try the 9-nightly build.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 25, 2019

My point is whether the incompatibility is recognized by team and tracked anywhere for fix until node v13 upgrade.

@cyfrost
Copy link

cyfrost commented Dec 25, 2019

Is there a temporary workaround to this? I'm unable to compile a typescript project due to the error listed in OP.

@amirburbea
Copy link

amirburbea commented Dec 25, 2019 via email

@cyfrost
Copy link

cyfrost commented Dec 25, 2019

Force all references in your package.json and yarn.lock to use @types/node 12.21.12. This is an unacceptable but hopefully temporary scenario There's a few people trying to act as though the problem is electron 7 uses node 12 and not 13. They're just plain wrong. The problem is the use of extends vs implements keywords on classes/interfaces. Again you have version ranges (for example, redux says @types/node:^8)

On Wed, Dec 25, 2019, 8:33 AM Cyrus Frost @.***> wrote: Is there a temporary workaround to this? I'm unable to compile a typescript project due to the error listed in OP. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#21612?email_source=notifications&email_token=AAQGPCWYY4R2NEQ6OW23OYLQ2NOJLA5CNFSM4J63DZ52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHULURI#issuecomment-568900165>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQGPCTIGMVJ4QSYKYF7TKTQ2NOJLANCNFSM4J63DZ5Q .

This workaround works! Thanks! Minor correction: the @types/node version in package.json should be 12.12.21 instead of 12.21.12.

@vladimiry
Copy link

Looks like we got thoughtful TypeSript and semver versioning expert here ==> @amirburbea

The problem is the use of
extends vs implements keywords on classes/interfaces.

Electron npm module is shipped with @types/node@^12.0.12 at the moment so it's totally fine that they do extends NodeJS.EventEmitter since NodeJS.EventEmitter is a class in @types/node prior v13.

It's expected as Electron v7 comes with node v12, so use the most recent @types/node v12.

No @vladimiry . That's not how it works

Electron even has automated test added for verifying that major versions of bundled node and @types/node got matched, see https://github.com/electron/electron/blob/f426ad1b5914295f9684036fb55f918c1b074900/spec-main/types-spec.ts

there is no direct tie between @types/node and the version of nodejs

There's a few people trying to act as though the problem is electron 7 uses
node 12 and not 13.

For curious persons, checking @types/node and nodejs versions correlation:

You can clone the https://github.com/DefinitelyTyped/DefinitelyTyped and execute git log -L 1,1:types/node/index.d.ts --pretty=oneline to see that commit message corresponds to first-line/header of the types/node/index.d.ts file change. So this pretty much indicates that version of @types/node corresponds to nodejs version.

c47a34ead1637f6f34e7d630dc88ea3f6e5562cb feat(node): v13 (#40927)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 12.12
+// Type definitions for non-npm package Node.js 13.1
a91a9a49ded5dd536a4bb0d2098eb1b0de1eadcd feat(node): v12.12 (#39914)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 12.11
+// Type definitions for non-npm package Node.js 12.12
cbe21d594a3b24fd1b93719958bc90ab6007ac51 feat(node): v12.11 (#39116)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 12.7
+// Type definitions for non-npm package Node.js 12.11
3f6627ea70b804317b90c0212b2055eff0a67a6f feat(node): v12.7 (#37217)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 12.6
+// Type definitions for non-npm package Node.js 12.7
4db4c26549c7edd452c46492f3421d5dce3200b0 feat: node v12.5 (#36562)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 12.0
+// Type definitions for non-npm package Node.js 12.6
18b13d1f4a6830861bf4f693a706dcde4aa8b21f feat(node): v12 (#34952)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 11.13
+// Type definitions for non-npm package Node.js 12.0
e6625ba155c7529e8bfb751a23b0d12bb8fe1ec5 feat(node): v11.13

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 11.12
+// Type definitions for non-npm package Node.js 11.13
923fe86451333613c015358a7447ed914945427f feat(node): v11.12 (#33967)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 11.11
+// Type definitions for non-npm package Node.js 11.12
bac137341280d31ee0f8159ddad66b0fa144e5fa feat(node): v11.11

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 11.10
+// Type definitions for non-npm package Node.js 11.11
80fb3119f40c6b70e4a9632887980036e36f6b2b feat(node): v11.10

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for non-npm package Node.js 11.9
+// Type definitions for non-npm package Node.js 11.10
608c146d8982a2ff7307f0a3bdddd6d55cb1a63e Mark non-npm packages

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 11.9
+// Type definitions for non-npm package Node.js 11.9
c8dadc971b8524704c4b39243780e294d7a15f12 feat(node): enable strict null checks and add SharedArrayBuffer

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 11.6
+// Type definitions for Node.js 11.9
e69abd8af13b4dd17259e72898b118102993915d feat(node): v11.6

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.12
+// Type definitions for Node.js 11.6
df80e09009547e5556c09a16f3c715ecf9aff325 feat(node): 10.12 (#29689)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.11
+// Type definitions for Node.js 10.12
352f2a6a39758bead6182a0d04bcf0b8d7c7a381 chore(node): apply lint

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.11.x
+// Type definitions for Node.js 10.11
e17d39e6b09a1e44d7dd69fa22b3da42c7156948 feat(node): 10.11

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.10.x
+// Type definitions for Node.js 10.11.x
9061b19a3c7d676fc4aeacc825ab7790edad13c6 types(node): add changes from 10.10 (#28918)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.9.x
+// Type definitions for Node.js 10.10.x
374598de675780b1312912d43dff88f9fbfcf0d8 [node] Add http[s] request/get variants for 10.9.0 (#28193)

diff --git a/types/node/index.d.ts b/types/node/index.d.ts
--- a/types/node/index.d.ts
+++ b/types/node/index.d.ts
@@ -1,1 +1,1 @@
-// Type definitions for Node.js 10.7.x
+// Type definitions for Node.js 10.9.x
3c8f15080c5728847c627ba07a623276ef1869ab add timeout to http.AgentOptions (#21346 
...

Regarding @types/node v13. You could see that what they do there with like 160 d.ts files of different libraries to make them compatible with @types/node@^13 is replacing

declare class SomeClass extends NodeJS.EventEmitter

with

import { EventEmitter } from 'events'; 
declare class SomeClass extends EventEmitter { 

So @electron will probably follow the same way one day when they move to @types/node@^13.

CC @MarshallOfSound

@rhysd
Copy link
Contributor Author

rhysd commented Dec 26, 2019

I believe replacing <reference/> with import/export can start without waiting for Node.js v13 upgrade of Electron since the replacement is also valid for @types/node v12.

I concern this because it requires many changes in electron.d.ts. Currently import statement is not available in electron.d.ts due to <reference> and namespace usage. So we can't simply replace NodeJS.EventEmitter with import { EventEmitter } from 'events'. As I said at first comment of this issue, at first it is necessary to remove <reference> and add export to all public APIs for use of import statement. These would be breaking changes in electron.d.ts so we must add them at major version up of electron package.

@andrewbranch
Copy link

From the DefinitelyTyped side, I’ve confirmed this was a correct change. The typings previously allowed this:

new NodeJS.EventEmitter();

which of course does not exist at runtime.

Currently import statement is not available in electron.d.ts due to <reference> and namespace usage.

Sort of—the deal is that when you have any import or export declaration in a file, that file becomes a module, so declarations are no longer global. If you need global declarations and imported stuff in the same file, you can solve this in two ways:

  1. Import what you want (so your file becomes a module), then wrap global declarations in a declare global { ... } block.
  2. Instead of doing import { EventEmitter } from 'events', you can use type EventEmitter = import('events').EventEmitter. But, this syntax is only available in TypeScript 2.9 and later.

@amirburbea
Copy link

I think it's safe to assume if someone is using a recent version of @types/node, they're probably using a semi recent version of typescript (2.9 came out over a year ago now)

@andrewbranch
Copy link

That’s not really the issue—the decision for electron to make is whether they need to support anyone with TS 2.8 or earlier, because they’re presumably only going to ship the same electron.d.ts to everyone. They can’t ship the new syntax conditionally upon whether the user has a recent @types/node.

@amirburbea
Copy link

amirburbea commented Dec 30, 2019

I think it's safe to move to 3.0 as a baseline for electron's use of typescript. That would be 18 months old already. (TS3.0 released July2018, TS 2.8 was March2018 - not even a huge lag time between them)

@andrewbranch
Copy link

I have no real opinion on how this should be dealt with, but as the person who merged the PR that caused this issue, I just wanted to

  • let the electron maintainers know that this was an intentional change from @types/node and it’s probably not going to go away
  • offer potential methods for fixing it on their side

@andrewbranch
Copy link

andrewbranch commented Dec 30, 2019

Oh, and I totally missed the fact that there is a global events. So yeah, replacing NodeJS.EventEmitter with events.EventEmitter is even simpler, which @amirburbea already mentioned. Sorry to miss that 👍

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

I did not know that the dynamic import syntax is available in ambient context. Thank you for letting me know!

Yeah, global { ... } block is already used in @types/node v12 actually. So it should be safe to use it in electron.d.ts. I think we can choose easier way type EventEmitter = import('events').EventEmitter.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

I had an experiment that adds type EventEmitter = import('events').EventEmitter; in namespace Electron { ... } block and removes NodeJS. from class BrowserWindow extends NodeJS.EventEmitter. However, I got following error. I think type EventEmitter = ... only imports type but extends requires a value so it is unavailable here.

node_modules/electron/electron.d.ts:1657:31 - error TS2693: 'EventEmitter' only refers to a type, but is being used as a value here.

1657   class BrowserWindow extends EventEmitter {
                                   ~~~~~~~~~~~~

@amirburbea
Copy link

amirburbea commented Dec 31, 2019 via email

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

It does not work because import(...) syntax returns Promise value.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

So yeah, replacing NodeJS.EventEmitter with events.EventEmitter is even simpler, which @amirburbea already mentioned.

Where is the events global value defined? I simply replaced NodeJS.EventEmitter with events.EventEmitter but got following error:

node_modules/electron/electron.d.ts:1655:31 - error TS2552: Cannot find name 'events'. Did you mean 'Event'?

1655   class BrowserWindow extends events.EventEmitter {
                                   ~~~~~~

@amirburbea
Copy link

amirburbea commented Dec 31, 2019 via email

@rhysd
Copy link
Contributor Author

rhysd commented Dec 31, 2019

Yeah, so I think we need to choose the first option @andrewbranch showed. As I commented above, import statement is not available in current electron.d.ts because it is not a module. To use it, we need to remove <reference> and namespace and need to use export instead. It is a breaking change of electron.d.ts so timing is important.

@saostad
Copy link

saostad commented Apr 18, 2020

I still have this problem. I can't keep my @types/node to 12.12.21 for long time.
could you please reopen the issue and fix it?

@vladimiry
Copy link

I recommended before to open a new issue since in my experience closed issues don't get reopened here.

@skorunka
Copy link

skorunka commented May 5, 2020

Anyway how to make it working? Having downgraded "@types/jest" to "25.1.4" is not long-term fix :(. Thank you.

@vladimiry
Copy link

Right now only electron v10 is shipped with updated definitions (it uses @electron/typescript-definitions@^8.7.2).

@dropsonic
Copy link

Run npm install --save --save-exact @types/node@^12.12.6. It works afterward (even with Electron v8.2.5).

@vladimiry
Copy link

It works afterward (even with Electron v8.2.5).

Of course it works this way. The point of this issue is that other deps used in the project require node@^13.

@amirburbea
Copy link

Thank you @vladimiry, I've been following this thread for 4 or so months and every once in a while someone feels compelled to write how they got this to work by using @types/node 12 with some npm or yarn command. As you said, that's the point of this whole thread.

@saostad
Copy link

saostad commented May 21, 2020

same problem even with electron v9.0.0

@saostad
Copy link

saostad commented May 21, 2020

guys this is closed issue and nobody will look at it.
I oped another issue please add your comments on that.

@rhysd
Copy link
Contributor Author

rhysd commented May 22, 2020

As it is said repeatedly in this issue thread, the fix for electron.d.ts for Node v13 is already there. I had made a PR and it was merged. But Node.js integrated to Electron is still v12 even in Electron v9. So using @types/node@12 is proper and the fix is not included in Electron v9. You should use @types/node@12.

kei10in added a commit to kei10in/electron-webpack-typescript-react-redux-toolkit-quick-start that referenced this issue Jun 27, 2020
jks-liu added a commit to jks-liu/sipi that referenced this issue Jul 15, 2020
1674:31 Cannot extend an interface 'NodeJS.EventEmitter'. Did you mean 'implements'?
1672 | }
1673 |
> 1674 | class BrowserWindow extends NodeJS.EventEmitter {

use node 12
https://github.com/ikuokuo/start-electron/blob/master/docs/tutorial/5_electron-app.md
why not use node > 12
electron/electron#21612
namikingsoft added a commit to namikingsoft/interv-timer that referenced this issue Aug 23, 2020
@StationedInTheField
Copy link

StationedInTheField commented Aug 28, 2020

So I have the same problem trying to use fs in my Angular9/Electron9 desktop app, seems to me like the best workaround would be to require all the necessary classes (in my case electron.remote) in a preload.js instead of downgrading the node version just for a single module (electron)?

@saostad
Copy link

saostad commented Aug 28, 2020

upgrade to electron 10, problem solved!

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

Successfully merging a pull request may close this issue.