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

Class static side 'typeof Emitter' incorrectly extends base class static side 'typeof EventEmitter' #385

Closed
moltar opened this issue Feb 17, 2021 · 9 comments · Fixed by #387

Comments

@moltar
Copy link

moltar commented Feb 17, 2021

Describe the Bug

When building the project with TS, I get the following error:

node_modules/cloudevents/dist/transport/emitter.d.ts:48:22 - error TS2417: Class static side 'typeof Emitter' incorrectly extends base class static side 'typeof EventEmitter'.
  Types of property 'on' are incompatible.
    Type '(event: "newListener" | "removeListener" | "cloudevent", listener: (...args: any[]) => void) => void' is not assignable to type '(emitter: EventEmitter, event: string) => AsyncIterableIterator<any>'.
      Types of parameters 'event' and 'emitter' are incompatible.
        Type 'EventEmitter' is not assignable to type '"newListener" | "removeListener" | "cloudevent"'.
          Type 'EventEmitter' is not assignable to type '"cloudevent"'.

48 export declare class Emitter extends EventEmitter {
                       ~~~~~~~
    "cloudevents": "4.0.0",
    "@types/node": "14.14.27",
    "typescript": "4.1.3"

Steps to Reproduce

N/A

Expected Behavior

Build passing.

Additional context

    "cloudevents": "4.0.0",
    "@types/node": "14.14.27",
    "typescript": "4.1.3"
@lance
Copy link
Member

lance commented Feb 18, 2021

Hi @moltar if you could provide some steps to reproduce this, that would be great! Looking at the typescript example in this repository, I do not see this problem. Thanks. :)

@moltar
Copy link
Author

moltar commented Feb 19, 2021

Yup, here you go:

https://github.com/moltar/cloudevents-exp

Thanks!

@hilfor
Copy link

hilfor commented Feb 23, 2021

+1

@mnesbitt
Copy link

I'm having this issue as well in version 4.0.0.

I did some testing and it looks like it's caused by the version of @types/node being used. The typescript example in this repository is using @types/[email protected] which is 3 years old. I'm currently using @types/[email protected] and seeing the issue, and @moltar 's example is using 14.14 as well.

I downgraded to [email protected] for now and it's compiling for me with the latest @types/node.

@lance
Copy link
Member

lance commented Feb 25, 2021

@mnesbitt thanks for that info. I'll take a look.

lance added a commit that referenced this issue Feb 25, 2021
This change modifies Emitter so that it does not directly extend the Node.js
EventEmitter class. Instead, it holds a singleton instance of an EventEmitter
but is not an instance of EventEmitter itself.

This commit also updates the typescript example to use a modern version of
@types/node and typescript.

Finally there are a few minor formatting changes picked up by eslint.

Fixes: #385

Signed-off-by: Lance Ball <[email protected]>
@lance
Copy link
Member

lance commented Feb 25, 2021

@mnesbitt @moltar @hilfor if you all have an opportunity to test this change in your project and provide feedback, that would be appreciated. Thanks!

@moltar
Copy link
Author

moltar commented Feb 26, 2021

@lance can we get a pre-release or an alpha channel release? It's not easy to test this, because the code needs to be transpiled to be recognized by NPM.

I installed it from the PR:

npm install cloudevents/sdk-javascript#pull/387/head

But the dir does not have the packaged files:

❯ tree node_modules/cloudevents
node_modules/cloudevents
├── CHANGELOG.md
├── LICENSE
├── README.md
└── package.json

@moltar
Copy link
Author

moltar commented Feb 26, 2021

lance added a commit that referenced this issue Feb 28, 2021
This change modifies Emitter so that it does not directly extend the Node.js
EventEmitter class. Instead, it holds a singleton instance of an EventEmitter
but is not an instance of EventEmitter itself.

This commit also updates the typescript example to use a modern version of
@types/node and typescript.

Finally there are a few minor formatting changes picked up by eslint.

Fixes: #385

Signed-off-by: Lance Ball <[email protected]>
@lance
Copy link
Member

lance commented Mar 3, 2021

@moltar thanks for bringing this up.

Given that this is a community repo with established processes, we'll need to discuss it. But I think it is a good suggestion. I've opened an issue to track this #388.

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 a pull request may close this issue.

4 participants