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

Typing of 3 param version of fromEvent in v7 is not compatible with EventEmitter of node.js #6301

Closed
thynson opened this issue Apr 30, 2021 · 6 comments

Comments

@thynson
Copy link

thynson commented Apr 30, 2021

Bug Report

Current Behavior
Typing of fromEvent in v7 is not compatible with EventEmitter of node.js, while its declaration claims it's accepted.

Expected behavior
It shall be compatible with v6.

Reproduction

fromEvent(process, 'SIGINT', {once: true}); // Does not compile

Environment

  • Runtime: Node v14
  • RxJS version: 7

Actually, it's Typescript that does not resolve function overload properly when the function signature contains union type, but at least Typescript is still able to resolve overload by the number of arguments, which is why declaration in v6 works.

@thynson
Copy link
Author

thynson commented Apr 30, 2021

Caused by #6214

@cartant
Copy link
Collaborator

cartant commented Apr 30, 2021

Needs a minimal reproduction. This seems to be fine, for me with the versions you mentioned:

import { fromEvent } from "rxjs";
const beforeExit = fromEvent(process, "beforeExit");

@thynson
Copy link
Author

thynson commented Apr 30, 2021

Sorry, I imported an EventEmitter that is not from 'node' when trying to craft a minimal repo.
But

fromEvent(process, 'SIGINT', {once: true});

does not compile.

@thynson thynson changed the title Typing of fromEvent in v7 is not compatible with EventEmitter of node.js Typing of 3 param version of fromEvent in v7 is not compatible with EventEmitter of node.js Apr 30, 2021
@cartant
Copy link
Collaborator

cartant commented Apr 30, 2021

AFAICT, this doesn't compile using the Node types:

process.addListener("SIGINT", { once: true });

And the Node documentation makes no mention of an options parameter, so I don't see why we should pass it.

If you were relying on this in v6, I am not at all sure that it would have been doing what you thought it was. This is the v6 implementation and it does not pass options to a Node-style target.

const source = sourceObj;
sourceObj.addListener(eventName, handler as NodeEventHandler);
unsubscribe = () => source.removeListener(eventName, handler as NodeEventHandler);

@thynson
Copy link
Author

thynson commented Apr 30, 2021

Umm, the original intent in my codebase was to subscribe to the event for once, so the {once: true} is a misuse and shall be fixed by .pipe(first()).

@thynson
Copy link
Author

thynson commented Apr 30, 2021

And the 3-param overload of fromEvent for node's EventEmitter is invalid and shall be removed.

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

No branches or pull requests

2 participants